* [PATCH 0/2] v2: F_OFD_GETLK extension to read lock info
@ 2023-06-21 15:22 Stas Sergeev
2023-06-21 15:22 ` [PATCH 1/2] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Stas Sergeev @ 2023-06-21 15:22 UTC (permalink / raw
To: linux-kernel
Cc: Stas Sergeev, Jeff Layton, Chuck Lever, Alexander Viro,
Christian Brauner, linux-fsdevel, Shuah Khan, linux-kselftest,
linux-api
This extension allows to use F_UNLCK on query, which currently returns
EINVAL. Instead it can be used to query the locks on a particular fd -
something that is not currently possible. The basic idea is that on
F_OFD_GETLK, F_UNLCK would "conflict" with (or query) any types of the
lock on the same fd, and ignore any locks on other fds.
Use-cases:
1. CRIU-alike scenario when you want to read the locking info from an
fd for the later reconstruction. This can now be done by setting
l_start and l_len to 0 to cover entire file range, and do F_OFD_GETLK.
In the loop you need to advance l_start past the returned lock ranges,
to eventually collect all locked ranges.
2. Implementing the lock checking/enforcing policy.
Say you want to implement an "auditor" module in your program,
that checks that the I/O is done only after the proper locking is
applied on a file region. In this case you need to know if the
particular region is locked on that fd, and if so - with what type
of the lock. If you would do that currently (without this extension)
then you can only check for the write locks, and for that you need to
probe the lock on your fd and then open the same file via another fd and
probe there. That way you can identify the write lock on a particular
fd, but such trick is non-atomic and complex. As for finding out the
read lock on a particular fd - impossible.
This extension allows to do such queries without any extra efforts.
3. Implementing the mandatory locking policy.
Suppose you want to make a policy where the write lock inhibits any
unlocked readers and writers. Currently you need to check if the
write lock is present on some other fd, and if it is not there - allow
the I/O operation. But because the write lock can appear at any moment,
you need to do that under some global lock, which can be released only
when the I/O operation is finished.
With the proposed extension you can instead just check the write lock
on your own fd first, and if it is there - allow the I/O operation on
that fd without using any global lock. Only if there is no write lock
on this fd, then you need to take global lock and check for a write
lock on other fds.
The second patch adds a test-case for OFD locks.
It tests both the generic things and the proposed extension.
The third patch is a proposed man page update for fcntl(2)
(not for the linux source tree)
Changes in v2:
- Dropped the l_pid extension patch and updated test-case accordingly.
Stas Sergeev (2):
fs/locks: F_UNLCK extension for F_OFD_GETLK
selftests: add OFD lock tests
fs/locks.c | 23 +++-
tools/testing/selftests/locking/Makefile | 2 +
tools/testing/selftests/locking/ofdlocks.c | 132 +++++++++++++++++++++
3 files changed, 154 insertions(+), 3 deletions(-)
create mode 100644 tools/testing/selftests/locking/ofdlocks.c
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Shuah Khan <shuah@kernel.org>
CC: linux-kselftest@vger.kernel.org
CC: linux-api@vger.kernel.org
--
2.39.2
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] fs/locks: F_UNLCK extension for F_OFD_GETLK
2023-06-21 15:22 [PATCH 0/2] v2: F_OFD_GETLK extension to read lock info Stas Sergeev
@ 2023-06-21 15:22 ` Stas Sergeev
2023-06-21 15:22 ` [PATCH 2/2] selftests: add OFD lock tests Stas Sergeev
2023-06-21 15:22 ` [PATCH] fcntl.2: document F_UNLCK F_OFD_GETLK extension Stas Sergeev
2 siblings, 0 replies; 16+ messages in thread
From: Stas Sergeev @ 2023-06-21 15:22 UTC (permalink / raw
To: linux-kernel
Cc: Stas Sergeev, Jeff Layton, Chuck Lever, Alexander Viro,
Christian Brauner, linux-fsdevel, Shuah Khan, linux-kselftest,
linux-api
Currently F_UNLCK with F_OFD_GETLK returns -EINVAL.
The proposed extension allows to use it for getting the lock
information from the particular fd.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Shuah Khan <shuah@kernel.org>
CC: linux-kselftest@vger.kernel.org
CC: linux-api@vger.kernel.org
---
fs/locks.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index df8b26a42524..210766007e63 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -868,6 +868,21 @@ static bool posix_locks_conflict(struct file_lock *caller_fl,
return locks_conflict(caller_fl, sys_fl);
}
+/* Determine if lock sys_fl blocks lock caller_fl. Used on xx_GETLK
+ * path so checks for additional GETLK-specific things like F_UNLCK.
+ */
+static bool posix_test_locks_conflict(struct file_lock *caller_fl,
+ struct file_lock *sys_fl)
+{
+ /* F_UNLCK checks any locks on the same fd. */
+ if (caller_fl->fl_type == F_UNLCK) {
+ if (!posix_same_owner(caller_fl, sys_fl))
+ return false;
+ return locks_overlap(caller_fl, sys_fl);
+ }
+ return posix_locks_conflict(caller_fl, sys_fl);
+}
+
/* Determine if lock sys_fl blocks lock caller_fl. FLOCK specific
* checking before calling the locks_conflict().
*/
@@ -901,7 +916,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
retry:
spin_lock(&ctx->flc_lock);
list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
- if (!posix_locks_conflict(fl, cfl))
+ if (!posix_test_locks_conflict(fl, cfl))
continue;
if (cfl->fl_lmops && cfl->fl_lmops->lm_lock_expirable
&& (*cfl->fl_lmops->lm_lock_expirable)(cfl)) {
@@ -2207,7 +2222,8 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
if (fl == NULL)
return -ENOMEM;
error = -EINVAL;
- if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
+ if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
+ && flock->l_type != F_WRLCK)
goto out;
error = flock_to_posix_lock(filp, fl, flock);
@@ -2414,7 +2430,8 @@ int fcntl_getlk64(struct file *filp, unsigned int cmd, struct flock64 *flock)
return -ENOMEM;
error = -EINVAL;
- if (flock->l_type != F_RDLCK && flock->l_type != F_WRLCK)
+ if (cmd != F_OFD_GETLK && flock->l_type != F_RDLCK
+ && flock->l_type != F_WRLCK)
goto out;
error = flock64_to_posix_lock(filp, fl, flock);
--
2.39.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] selftests: add OFD lock tests
2023-06-21 15:22 [PATCH 0/2] v2: F_OFD_GETLK extension to read lock info Stas Sergeev
2023-06-21 15:22 ` [PATCH 1/2] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev
@ 2023-06-21 15:22 ` Stas Sergeev
2023-06-22 11:48 ` Jeff Layton
2023-06-21 15:22 ` [PATCH] fcntl.2: document F_UNLCK F_OFD_GETLK extension Stas Sergeev
2 siblings, 1 reply; 16+ messages in thread
From: Stas Sergeev @ 2023-06-21 15:22 UTC (permalink / raw
To: linux-kernel
Cc: Stas Sergeev, Jeff Layton, Chuck Lever, Alexander Viro,
Christian Brauner, linux-fsdevel, Shuah Khan, linux-kselftest,
linux-api
Test the basic locking stuff on 2 fds: multiple read locks,
conflicts between read and write locks, use of len==0 for queries.
Also tests for F_UNLCK F_OFD_GETLK extension.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Shuah Khan <shuah@kernel.org>
CC: linux-kselftest@vger.kernel.org
CC: linux-api@vger.kernel.org
---
tools/testing/selftests/locking/Makefile | 2 +
tools/testing/selftests/locking/ofdlocks.c | 132 +++++++++++++++++++++
2 files changed, 134 insertions(+)
create mode 100644 tools/testing/selftests/locking/ofdlocks.c
diff --git a/tools/testing/selftests/locking/Makefile b/tools/testing/selftests/locking/Makefile
index 6e7761ab3536..a83ced1626de 100644
--- a/tools/testing/selftests/locking/Makefile
+++ b/tools/testing/selftests/locking/Makefile
@@ -7,4 +7,6 @@ all:
TEST_PROGS := ww_mutex.sh
+TEST_GEN_PROGS := ofdlocks
+
include ../lib.mk
diff --git a/tools/testing/selftests/locking/ofdlocks.c b/tools/testing/selftests/locking/ofdlocks.c
new file mode 100644
index 000000000000..1ccb2b9b5ead
--- /dev/null
+++ b/tools/testing/selftests/locking/ofdlocks.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <fcntl.h>
+#include <assert.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <string.h>
+#include "../kselftest.h"
+
+static int lock_set(int fd, struct flock *fl)
+{
+ int ret;
+
+ fl->l_pid = 0; // needed for OFD locks
+ fl->l_whence = SEEK_SET;
+ ret = fcntl(fd, F_OFD_SETLK, fl);
+ if (ret)
+ perror("fcntl()");
+ return ret;
+}
+
+static int lock_get(int fd, struct flock *fl)
+{
+ int ret;
+
+ fl->l_pid = 0; // needed for OFD locks
+ fl->l_whence = SEEK_SET;
+ ret = fcntl(fd, F_OFD_GETLK, fl);
+ if (ret)
+ perror("fcntl()");
+ return ret;
+}
+
+int main(void)
+{
+ int rc;
+ struct flock fl, fl2;
+ int fd = open("/tmp/aa", O_RDWR | O_CREAT | O_EXCL, 0600);
+ int fd2 = open("/tmp/aa", O_RDONLY);
+
+ unlink("aa");
+ assert(fd != -1);
+ assert(fd2 != -1);
+ ksft_print_msg("[INFO] opened fds %i %i\n", fd, fd2);
+
+ /* Set some read lock */
+ fl.l_type = F_RDLCK;
+ fl.l_start = 5;
+ fl.l_len = 3;
+ rc = lock_set(fd, &fl);
+ if (rc == 0) {
+ ksft_print_msg
+ ("[SUCCESS] set OFD read lock on first fd\n");
+ } else {
+ ksft_print_msg("[FAIL] to set OFD read lock on first fd\n");
+ return -1;
+ }
+ /* Make sure read locks do not conflict on different fds. */
+ fl.l_type = F_RDLCK;
+ fl.l_start = 5;
+ fl.l_len = 1;
+ rc = lock_get(fd2, &fl);
+ if (rc != 0)
+ return -1;
+ if (fl.l_type != F_UNLCK) {
+ ksft_print_msg("[FAIL] read locks conflicted\n");
+ return -1;
+ }
+ /* Make sure read/write locks do conflict on different fds. */
+ fl.l_type = F_WRLCK;
+ fl.l_start = 5;
+ fl.l_len = 1;
+ rc = lock_get(fd2, &fl);
+ if (rc != 0)
+ return -1;
+ if (fl.l_type != F_UNLCK) {
+ ksft_print_msg
+ ("[SUCCESS] read and write locks conflicted\n");
+ } else {
+ ksft_print_msg
+ ("[SUCCESS] read and write locks not conflicted\n");
+ return -1;
+ }
+ /* Get info about the lock on first fd. */
+ fl.l_type = F_UNLCK;
+ fl.l_start = 5;
+ fl.l_len = 1;
+ rc = lock_get(fd, &fl);
+ if (rc != 0) {
+ ksft_print_msg
+ ("[FAIL] F_OFD_GETLK with F_UNLCK not supported\n");
+ return -1;
+ }
+ if (fl.l_type != F_UNLCK) {
+ ksft_print_msg
+ ("[SUCCESS] F_UNLCK test returns: locked, type %i pid %i len %zi\n",
+ fl.l_type, fl.l_pid, fl.l_len);
+ } else {
+ ksft_print_msg
+ ("[FAIL] F_OFD_GETLK with F_UNLCK did not return lock info\n");
+ return -1;
+ }
+ /* Try the same but by locking everything by len==0. */
+ fl2.l_type = F_UNLCK;
+ fl2.l_start = 0;
+ fl2.l_len = 0;
+ rc = lock_get(fd, &fl2);
+ if (rc != 0) {
+ ksft_print_msg
+ ("[FAIL] F_OFD_GETLK with F_UNLCK not supported\n");
+ return -1;
+ }
+ if (memcmp(&fl, &fl2, sizeof(fl))) {
+ ksft_print_msg
+ ("[FAIL] F_UNLCK test returns: locked, type %i pid %i len %zi\n",
+ fl.l_type, fl.l_pid, fl.l_len);
+ return -1;
+ }
+ ksft_print_msg("[SUCCESS] F_UNLCK with len==0 returned the same\n");
+ /* Get info about the lock on second fd - no locks on it. */
+ fl.l_type = F_UNLCK;
+ fl.l_start = 0;
+ fl.l_len = 0;
+ lock_get(fd2, &fl);
+ if (fl.l_type != F_UNLCK) {
+ ksft_print_msg
+ ("[FAIL] F_OFD_GETLK with F_UNLCK return lock info from another fd\n");
+ return -1;
+ }
+ return 0;
+}
--
2.39.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH] fcntl.2: document F_UNLCK F_OFD_GETLK extension
2023-06-21 15:22 [PATCH 0/2] v2: F_OFD_GETLK extension to read lock info Stas Sergeev
2023-06-21 15:22 ` [PATCH 1/2] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev
2023-06-21 15:22 ` [PATCH 2/2] selftests: add OFD lock tests Stas Sergeev
@ 2023-06-21 15:22 ` Stas Sergeev
2023-06-22 12:03 ` Jeff Layton
2 siblings, 1 reply; 16+ messages in thread
From: Stas Sergeev @ 2023-06-21 15:22 UTC (permalink / raw
To: linux-kernel
Cc: Stas Sergeev, Jeff Layton, Chuck Lever, Alexander Viro,
Christian Brauner, linux-fsdevel, Shuah Khan, linux-kselftest,
linux-api
F_UNLCK has the special meaning when used as a lock type on input.
It returns the information about any lock found in the specified
region on that particular file descriptor. Locks on other file
descriptors are ignored by F_UNLCK.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Shuah Khan <shuah@kernel.org>
CC: linux-kselftest@vger.kernel.org
CC: linux-api@vger.kernel.org
---
man2/fcntl.2 | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/man2/fcntl.2 b/man2/fcntl.2
index 7b5604e3a..e3e3e7b8c 100644
--- a/man2/fcntl.2
+++ b/man2/fcntl.2
@@ -604,6 +604,13 @@ then details about one of these locks are returned via
.IR lock ,
as described above for
.BR F_GETLK .
+.B F_UNLCK
+has the special meaning when put into
+.I l_type
+as an input. It returns the information about any lock in the specified
+range on that particular file descriptor. The locks on other file
+descriptors are ignored by
+.BR F_UNLCK .
.PP
In the current implementation,
.\" commit 57b65325fe34ec4c917bc4e555144b4a94d9e1f7
--
2.39.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-21 15:22 ` [PATCH 2/2] selftests: add OFD lock tests Stas Sergeev
@ 2023-06-22 11:48 ` Jeff Layton
2023-06-22 16:40 ` stsp
2023-06-22 16:54 ` stsp
0 siblings, 2 replies; 16+ messages in thread
From: Jeff Layton @ 2023-06-22 11:48 UTC (permalink / raw
To: Stas Sergeev, linux-kernel
Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel,
Shuah Khan, linux-kselftest, linux-api
On Wed, 2023-06-21 at 20:22 +0500, Stas Sergeev wrote:
> Test the basic locking stuff on 2 fds: multiple read locks,
> conflicts between read and write locks, use of len==0 for queries.
> Also tests for F_UNLCK F_OFD_GETLK extension.
>
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>
> CC: Jeff Layton <jlayton@kernel.org>
> CC: Chuck Lever <chuck.lever@oracle.com>
> CC: Alexander Viro <viro@zeniv.linux.org.uk>
> CC: Christian Brauner <brauner@kernel.org>
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: Shuah Khan <shuah@kernel.org>
> CC: linux-kselftest@vger.kernel.org
> CC: linux-api@vger.kernel.org
>
> ---
> tools/testing/selftests/locking/Makefile | 2 +
> tools/testing/selftests/locking/ofdlocks.c | 132 +++++++++++++++++++++
> 2 files changed, 134 insertions(+)
> create mode 100644 tools/testing/selftests/locking/ofdlocks.c
>
> diff --git a/tools/testing/selftests/locking/Makefile b/tools/testing/selftests/locking/Makefile
> index 6e7761ab3536..a83ced1626de 100644
> --- a/tools/testing/selftests/locking/Makefile
> +++ b/tools/testing/selftests/locking/Makefile
> @@ -7,4 +7,6 @@ all:
>
> TEST_PROGS := ww_mutex.sh
>
> +TEST_GEN_PROGS := ofdlocks
> +
> include ../lib.mk
I'm not sure this really belongs in the "locking" directory. Given that
there is only the ww_mutex test in there, that's more for internal
synchronization mechanisms, I think.
Can you create a new "filelock" directory and drop this into there
instead?
> diff --git a/tools/testing/selftests/locking/ofdlocks.c b/tools/testing/selftests/locking/ofdlocks.c
> new file mode 100644
> index 000000000000..1ccb2b9b5ead
> --- /dev/null
> +++ b/tools/testing/selftests/locking/ofdlocks.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include <fcntl.h>
> +#include <assert.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include "../kselftest.h"
> +
> +static int lock_set(int fd, struct flock *fl)
> +{
> + int ret;
> +
> + fl->l_pid = 0; // needed for OFD locks
> + fl->l_whence = SEEK_SET;
> + ret = fcntl(fd, F_OFD_SETLK, fl);
> + if (ret)
> + perror("fcntl()");
> + return ret;
> +}
> +
> +static int lock_get(int fd, struct flock *fl)
> +{
> + int ret;
> +
> + fl->l_pid = 0; // needed for OFD locks
> + fl->l_whence = SEEK_SET;
> + ret = fcntl(fd, F_OFD_GETLK, fl);
> + if (ret)
> + perror("fcntl()");
> + return ret;
> +}
> +
> +int main(void)
> +{
> + int rc;
> + struct flock fl, fl2;
> + int fd = open("/tmp/aa", O_RDWR | O_CREAT | O_EXCL, 0600);
> + int fd2 = open("/tmp/aa", O_RDONLY);
> +
> + unlink("aa");
> + assert(fd != -1);
> + assert(fd2 != -1);
> + ksft_print_msg("[INFO] opened fds %i %i\n", fd, fd2);
> +
> + /* Set some read lock */
> + fl.l_type = F_RDLCK;
> + fl.l_start = 5;
> + fl.l_len = 3;
> + rc = lock_set(fd, &fl);
> + if (rc == 0) {
> + ksft_print_msg
> + ("[SUCCESS] set OFD read lock on first fd\n");
> + } else {
> + ksft_print_msg("[FAIL] to set OFD read lock on first fd\n");
> + return -1;
> + }
> + /* Make sure read locks do not conflict on different fds. */
> + fl.l_type = F_RDLCK;
> + fl.l_start = 5;
> + fl.l_len = 1;
> + rc = lock_get(fd2, &fl);
> + if (rc != 0)
> + return -1;
> + if (fl.l_type != F_UNLCK) {
> + ksft_print_msg("[FAIL] read locks conflicted\n");
> + return -1;
> + }
> + /* Make sure read/write locks do conflict on different fds. */
> + fl.l_type = F_WRLCK;
> + fl.l_start = 5;
> + fl.l_len = 1;
> + rc = lock_get(fd2, &fl);
> + if (rc != 0)
> + return -1;
> + if (fl.l_type != F_UNLCK) {
> + ksft_print_msg
> + ("[SUCCESS] read and write locks conflicted\n");
> + } else {
> + ksft_print_msg
> + ("[SUCCESS] read and write locks not conflicted\n");
> + return -1;
> + }
> + /* Get info about the lock on first fd. */
> + fl.l_type = F_UNLCK;
> + fl.l_start = 5;
> + fl.l_len = 1;
> + rc = lock_get(fd, &fl);
> + if (rc != 0) {
> + ksft_print_msg
> + ("[FAIL] F_OFD_GETLK with F_UNLCK not supported\n");
> + return -1;
> + }
> + if (fl.l_type != F_UNLCK) {
> + ksft_print_msg
> + ("[SUCCESS] F_UNLCK test returns: locked, type %i pid %i len %zi\n",
> + fl.l_type, fl.l_pid, fl.l_len);
> + } else {
> + ksft_print_msg
> + ("[FAIL] F_OFD_GETLK with F_UNLCK did not return lock info\n");
> + return -1;
> + }
> + /* Try the same but by locking everything by len==0. */
> + fl2.l_type = F_UNLCK;
> + fl2.l_start = 0;
> + fl2.l_len = 0;
> + rc = lock_get(fd, &fl2);
> + if (rc != 0) {
> + ksft_print_msg
> + ("[FAIL] F_OFD_GETLK with F_UNLCK not supported\n");
> + return -1;
> + }
> + if (memcmp(&fl, &fl2, sizeof(fl))) {
> + ksft_print_msg
> + ("[FAIL] F_UNLCK test returns: locked, type %i pid %i len %zi\n",
> + fl.l_type, fl.l_pid, fl.l_len);
> + return -1;
> + }
> + ksft_print_msg("[SUCCESS] F_UNLCK with len==0 returned the same\n");
> + /* Get info about the lock on second fd - no locks on it. */
> + fl.l_type = F_UNLCK;
> + fl.l_start = 0;
> + fl.l_len = 0;
> + lock_get(fd2, &fl);
> + if (fl.l_type != F_UNLCK) {
> + ksft_print_msg
> + ("[FAIL] F_OFD_GETLK with F_UNLCK return lock info from another fd\n");
> + return -1;
> + }
> + return 0;
> +}
I'm not opposed to adding a selftest here, but most filesystem testing
is done via xfstests:
https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/
It would be better to add this test to the existing generic/478 test
that tests OFD locks. Can you patch that to add a test for the new
functionality?
Thanks,
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fcntl.2: document F_UNLCK F_OFD_GETLK extension
2023-06-21 15:22 ` [PATCH] fcntl.2: document F_UNLCK F_OFD_GETLK extension Stas Sergeev
@ 2023-06-22 12:03 ` Jeff Layton
2023-06-23 10:07 ` stsp
0 siblings, 1 reply; 16+ messages in thread
From: Jeff Layton @ 2023-06-22 12:03 UTC (permalink / raw
To: Stas Sergeev, linux-kernel
Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel,
Shuah Khan, linux-kselftest, linux-api
On Wed, 2023-06-21 at 20:22 +0500, Stas Sergeev wrote:
> F_UNLCK has the special meaning when used as a lock type on input.
> It returns the information about any lock found in the specified
> region on that particular file descriptor. Locks on other file
> descriptors are ignored by F_UNLCK.
>
> Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
>
> CC: Jeff Layton <jlayton@kernel.org>
> CC: Chuck Lever <chuck.lever@oracle.com>
> CC: Alexander Viro <viro@zeniv.linux.org.uk>
> CC: Christian Brauner <brauner@kernel.org>
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: Shuah Khan <shuah@kernel.org>
> CC: linux-kselftest@vger.kernel.org
> CC: linux-api@vger.kernel.org
>
> ---
> man2/fcntl.2 | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/man2/fcntl.2 b/man2/fcntl.2
> index 7b5604e3a..e3e3e7b8c 100644
> --- a/man2/fcntl.2
> +++ b/man2/fcntl.2
> @@ -604,6 +604,13 @@ then details about one of these locks are returned via
> .IR lock ,
> as described above for
> .BR F_GETLK .
> +.B F_UNLCK
> +has the special meaning when put into
> +.I l_type
> +as an input. It returns the information about any lock in the specified
> +range on that particular file descriptor. The locks on other file
> +descriptors are ignored by
> +.BR F_UNLCK .
> .PP
> In the current implementation,
> .\" commit 57b65325fe34ec4c917bc4e555144b4a94d9e1f7
We need to be pedantic for manpages. A "file description" is the
representation of the open file in the kernel (basically, the "struct
file" in the kernel). A file _descriptor_ is the numeric identifier
returned by open() and similar functions.
The locks are owned by the file description, so that would be the better
term to use here. I think you want something like:
"When the l_type is set to F_UNLCK, returned locks are limited to ones
set on the given file description. Locks set on other file descriptions
are ignored on F_GETLK requests with the l_type set to F_UNLCK."
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 11:48 ` Jeff Layton
@ 2023-06-22 16:40 ` stsp
2023-06-22 16:58 ` Chuck Lever III
2023-06-22 16:54 ` stsp
1 sibling, 1 reply; 16+ messages in thread
From: stsp @ 2023-06-22 16:40 UTC (permalink / raw
To: Jeff Layton, linux-kernel
Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel,
Shuah Khan, linux-kselftest, linux-api
22.06.2023 16:48, Jeff Layton пишет:
> I'm not opposed to adding a selftest here, but most filesystem testing
> is done via xfstests:
>
> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/
>
> It would be better to add this test to the existing generic/478 test
> that tests OFD locks. Can you patch that to add a test for the new
> functionality?
I don't actually think its possible.
It seems like their script creates 2
t_ofd_locks processes, one for creating
the lock, one for testing it.
This is not our case.
To make it work our way, we'd probably
need to hack that test directly into
t_ofd_locks.c, so that it can set and test
from the same fd. And I don't know how
to even run these tests. :)
So I am really inclined to limit myself
with a kernel selftest.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 11:48 ` Jeff Layton
2023-06-22 16:40 ` stsp
@ 2023-06-22 16:54 ` stsp
1 sibling, 0 replies; 16+ messages in thread
From: stsp @ 2023-06-22 16:54 UTC (permalink / raw
To: Jeff Layton, linux-kernel
Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel,
Shuah Khan, linux-kselftest, linux-api
22.06.2023 16:48, Jeff Layton пишет:
> I'm not sure this really belongs in the "locking" directory. Given that
> there is only the ww_mutex test in there, that's more for internal
> synchronization mechanisms, I think.
>
> Can you create a new "filelock" directory and drop this into there
> instead?
Done and sent the v3.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 16:40 ` stsp
@ 2023-06-22 16:58 ` Chuck Lever III
2023-06-22 17:05 ` stsp
2023-08-09 7:56 ` stsp
0 siblings, 2 replies; 16+ messages in thread
From: Chuck Lever III @ 2023-06-22 16:58 UTC (permalink / raw
To: stsp
Cc: Jeff Layton, linux-kernel@vger.kernel.org, Al Viro,
Christian Brauner, linux-fsdevel@vger.kernel.org, Shuah Khan,
linux-kselftest@vger.kernel.org, linux-api@vger.kernel.org
> On Jun 22, 2023, at 12:40 PM, stsp <stsp2@yandex.ru> wrote:
>
> 22.06.2023 16:48, Jeff Layton пишет:
>> I'm not opposed to adding a selftest here, but most filesystem testing
>> is done via xfstests:
>>
>> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/
>>
>> It would be better to add this test to the existing generic/478 test
>> that tests OFD locks. Can you patch that to add a test for the new
>> functionality?
I agree with Jeff, this test needs to go in xfstests.
> I don't actually think its possible.
> It seems like their script creates 2
> t_ofd_locks processes, one for creating
> the lock, one for testing it.
> This is not our case.
> To make it work our way, we'd probably
> need to hack that test directly into
> t_ofd_locks.c, so that it can set and test
> from the same fd.
Or you could add a new test program.
> And I don't know how
> to even run these tests. :)
> So I am really inclined to limit myself
> with a kernel selftest.
IMO that's not a reason not to do this properly.
You should work with Jeff and the maintainer of
xfstests to make it happen.
--
Chuck Lever
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 16:58 ` Chuck Lever III
@ 2023-06-22 17:05 ` stsp
2023-06-22 17:12 ` Chuck Lever III
2023-08-09 7:56 ` stsp
1 sibling, 1 reply; 16+ messages in thread
From: stsp @ 2023-06-22 17:05 UTC (permalink / raw
To: Chuck Lever III
Cc: Jeff Layton, linux-kernel@vger.kernel.org, Al Viro,
Christian Brauner, linux-fsdevel@vger.kernel.org, Shuah Khan,
linux-kselftest@vger.kernel.org, linux-api@vger.kernel.org
22.06.2023 21:58, Chuck Lever III пишет:
> IMO that's not a reason not to do this properly.
>
> You should work with Jeff and the maintainer of
> xfstests to make it happen.
But its not going to be in this patch-set
anyway, as its a different source tree...
So I should prepare it when this is merged,
or?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 17:05 ` stsp
@ 2023-06-22 17:12 ` Chuck Lever III
2023-06-22 17:31 ` stsp
2023-06-22 17:34 ` Frank Filz
0 siblings, 2 replies; 16+ messages in thread
From: Chuck Lever III @ 2023-06-22 17:12 UTC (permalink / raw
To: stsp
Cc: Jeff Layton, linux-kernel@vger.kernel.org, Al Viro,
Christian Brauner, linux-fsdevel@vger.kernel.org, Shuah Khan,
linux-kselftest@vger.kernel.org, linux-api@vger.kernel.org
> On Jun 22, 2023, at 1:05 PM, stsp <stsp2@yandex.ru> wrote:
>
>
> 22.06.2023 21:58, Chuck Lever III пишет:
>> IMO that's not a reason not to do this properly.
>>
>> You should work with Jeff and the maintainer of
>> xfstests to make it happen.
> But its not going to be in this patch-set
> anyway, as its a different source tree...
If others agree with me, then please drop the selftests
patch from this series. There is a considerably higher
probability that the new test will be run frequently
by CI if it's in xfstests.
> So I should prepare it when this is merged,
> or?
I don't have a strong preference. A good choice is to
push the test before the kernel changes are merged.
--
Chuck Lever
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 17:12 ` Chuck Lever III
@ 2023-06-22 17:31 ` stsp
2023-06-22 22:15 ` Dave Chinner
2023-06-22 17:34 ` Frank Filz
1 sibling, 1 reply; 16+ messages in thread
From: stsp @ 2023-06-22 17:31 UTC (permalink / raw
To: Chuck Lever III
Cc: Jeff Layton, linux-kernel@vger.kernel.org, Al Viro,
Christian Brauner, linux-fsdevel@vger.kernel.org, Shuah Khan,
linux-kselftest@vger.kernel.org, linux-api@vger.kernel.org
22.06.2023 22:12, Chuck Lever III пишет:
> I don't have a strong preference. A good choice is to
> push the test before the kernel changes are merged.
It will fail though w/o kernel changes.
So what exactly is the policy?
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 17:12 ` Chuck Lever III
2023-06-22 17:31 ` stsp
@ 2023-06-22 17:34 ` Frank Filz
1 sibling, 0 replies; 16+ messages in thread
From: Frank Filz @ 2023-06-22 17:34 UTC (permalink / raw
To: 'Chuck Lever III', 'stsp'
Cc: 'Jeff Layton', linux-kernel, 'Al Viro',
'Christian Brauner', linux-fsdevel, 'Shuah Khan',
linux-kselftest, linux-api
> > On Jun 22, 2023, at 1:05 PM, stsp <stsp2@yandex.ru> wrote:
> >
> >
> > 22.06.2023 21:58, Chuck Lever III пишет:
> >> IMO that's not a reason not to do this properly.
> >>
> >> You should work with Jeff and the maintainer of xfstests to make it
> >> happen.
> > But its not going to be in this patch-set anyway, as its a different
> > source tree...
>
> If others agree with me, then please drop the selftests patch from this series.
> There is a considerably higher probability that the new test will be run frequently
> by CI if it's in xfstests.
>
>
> > So I should prepare it when this is merged, or?
>
> I don't have a strong preference. A good choice is to push the test before the
> kernel changes are merged.
As an aside, an additional testing option for OFD locks is the multilock test tool that is in the nfs-ganesha project. In preparation to use OFD locks in Ganesha, I added them to multilock to check them out, and that incidentally also allows testing how the NFS client and server work when OFD locks are taken on a file from an NFS mount.
Frank
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 17:31 ` stsp
@ 2023-06-22 22:15 ` Dave Chinner
0 siblings, 0 replies; 16+ messages in thread
From: Dave Chinner @ 2023-06-22 22:15 UTC (permalink / raw
To: stsp
Cc: Chuck Lever III, Jeff Layton, linux-kernel@vger.kernel.org,
Al Viro, Christian Brauner, linux-fsdevel@vger.kernel.org,
Shuah Khan, linux-kselftest@vger.kernel.org,
linux-api@vger.kernel.org
On Thu, Jun 22, 2023 at 10:31:06PM +0500, stsp wrote:
>
> 22.06.2023 22:12, Chuck Lever III пишет:
> > I don't have a strong preference. A good choice is to
> > push the test before the kernel changes are merged.
> It will fail though w/o kernel changes.
> So what exactly is the policy?
filesystem unit test functionality needs to be pushed into fstests
and/or ltp. The preference is the former, because just about every
filesystem developer and distro QA team is running this as part of
their every-day testing workflow.
fstests is written to probe whether the kernel supports a given
feature or not before testing it. It will _not_run() a test that
doesn't have the required kernel/fs/device support, and this is not
considered a test failure.
Yes, it means you have to also write the userspace feature probing
code, but that should be trivial to do because userspace already has
to be able to safely discover that this extension exists, right?
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] fcntl.2: document F_UNLCK F_OFD_GETLK extension
2023-06-22 12:03 ` Jeff Layton
@ 2023-06-23 10:07 ` stsp
0 siblings, 0 replies; 16+ messages in thread
From: stsp @ 2023-06-23 10:07 UTC (permalink / raw
To: Jeff Layton, linux-kernel
Cc: Chuck Lever, Alexander Viro, Christian Brauner, linux-fsdevel,
Shuah Khan, linux-kselftest, linux-api
22.06.2023 17:03, Jeff Layton пишет:
> We need to be pedantic for manpages. A "file description" is the
> representation of the open file in the kernel (basically, the "struct
> file" in the kernel). A file _descriptor_ is the numeric identifier
> returned by open() and similar functions.
OK.
> The locks are owned by the file description, so that would be the better
> term to use here. I think you want something like:
>
> "When the l_type is set to F_UNLCK, returned locks are limited to ones
> set on the given file description.
This is also inaccurate, because "limited"
implies other operations act widely.
But actually other operations do not
consider the "same fd" at all. So the
reported sets by F_UNLCK and other
ops do not overlap. Which is why I
decided to describe it as a "special
meaning".
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] selftests: add OFD lock tests
2023-06-22 16:58 ` Chuck Lever III
2023-06-22 17:05 ` stsp
@ 2023-08-09 7:56 ` stsp
1 sibling, 0 replies; 16+ messages in thread
From: stsp @ 2023-08-09 7:56 UTC (permalink / raw
To: Chuck Lever III
Cc: Jeff Layton, linux-kernel@vger.kernel.org, Al Viro,
Christian Brauner, linux-fsdevel@vger.kernel.org, Shuah Khan,
linux-kselftest@vger.kernel.org, linux-api@vger.kernel.org
Hi,
22.06.2023 21:58, Chuck Lever III пишет:
> IMO that's not a reason not to do this properly.
>
> You should work with Jeff and the maintainer of
> xfstests to make it happen.
If only I knew who is that maintainer
and why he ignores all patches...
Can someone else please take a look at this:
https://www.spinics.net/lists/fstests/msg23115.html
and make a review/ack?
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-08-09 8:04 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-21 15:22 [PATCH 0/2] v2: F_OFD_GETLK extension to read lock info Stas Sergeev
2023-06-21 15:22 ` [PATCH 1/2] fs/locks: F_UNLCK extension for F_OFD_GETLK Stas Sergeev
2023-06-21 15:22 ` [PATCH 2/2] selftests: add OFD lock tests Stas Sergeev
2023-06-22 11:48 ` Jeff Layton
2023-06-22 16:40 ` stsp
2023-06-22 16:58 ` Chuck Lever III
2023-06-22 17:05 ` stsp
2023-06-22 17:12 ` Chuck Lever III
2023-06-22 17:31 ` stsp
2023-06-22 22:15 ` Dave Chinner
2023-06-22 17:34 ` Frank Filz
2023-08-09 7:56 ` stsp
2023-06-22 16:54 ` stsp
2023-06-21 15:22 ` [PATCH] fcntl.2: document F_UNLCK F_OFD_GETLK extension Stas Sergeev
2023-06-22 12:03 ` Jeff Layton
2023-06-23 10:07 ` stsp
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).