* [PATCH v3 0/2] fs/lock: Cleanup around flock syscall.
@ 2022-07-17 4:35 Kuniyuki Iwashima
2022-07-17 4:35 ` [PATCH v3 1/2] fs/lock: Don't allocate file_lock in flock_make_lock() Kuniyuki Iwashima
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Kuniyuki Iwashima @ 2022-07-17 4:35 UTC (permalink / raw
To: Jeff Layton, Chuck Lever, Alexander Viro
Cc: Kuniyuki Iwashima, Kuniyuki Iwashima, linux-fsdevel
The first patch removes allocate-and-free for struct file_lock
in flock syscall and the second patch rearrange some operations.
Changes:
v3:
* Test LOCK_MAND first in patch 2
v2: https://lore.kernel.org/linux-fsdevel/20220716233343.22106-1-kuniyu@amazon.com/
* Use F_UNLCK in locks_remove_flock() (Chuck Lever)
* Fix uninitialised error in flock syscall (kernel test robot)
* Fix error when setting LOCK_NB
* Split patches not to mix different kinds of optimisations and
not to miss errors reported by kernel test robot
v1: https://lore.kernel.org/linux-fsdevel/20220716013140.61445-1-kuniyu@amazon.com/
Kuniyuki Iwashima (2):
fs/lock: Don't allocate file_lock in flock_make_lock().
fs/lock: Rearrange ops in flock syscall.
fs/locks.c | 77 ++++++++++++++++++++----------------------------------
1 file changed, 28 insertions(+), 49 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/2] fs/lock: Don't allocate file_lock in flock_make_lock().
2022-07-17 4:35 [PATCH v3 0/2] fs/lock: Cleanup around flock syscall Kuniyuki Iwashima
@ 2022-07-17 4:35 ` Kuniyuki Iwashima
2022-07-18 13:54 ` Chuck Lever III
2022-07-17 4:35 ` [PATCH v3 2/2] fs/lock: Rearrange ops in flock syscall Kuniyuki Iwashima
2022-07-18 10:43 ` [PATCH v3 0/2] fs/lock: Cleanup around " Jeff Layton
2 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2022-07-17 4:35 UTC (permalink / raw
To: Jeff Layton, Chuck Lever, Alexander Viro
Cc: Kuniyuki Iwashima, Kuniyuki Iwashima, linux-fsdevel
Two functions, flock syscall and locks_remove_flock(), call
flock_make_lock(). It allocates struct file_lock from slab
cache if its argument fl is NULL.
When we call flock syscall, we pass NULL to allocate memory
for struct file_lock. However, we always free it at the end
by locks_free_lock(). We need not allocate it and instead
should use a local variable as locks_remove_flock() does.
Also, the validation for flock_translate_cmd() is not necessary
for locks_remove_flock(). So we move the part to flock syscall
and make flock_make_lock() return nothing.
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
---
fs/locks.c | 46 +++++++++++++++-------------------------------
1 file changed, 15 insertions(+), 31 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index ca28e0e50e56..b134eaefd7d6 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -425,21 +425,9 @@ static inline int flock_translate_cmd(int cmd) {
}
/* Fill in a file_lock structure with an appropriate FLOCK lock. */
-static struct file_lock *
-flock_make_lock(struct file *filp, unsigned int cmd, struct file_lock *fl)
+static void flock_make_lock(struct file *filp, struct file_lock *fl, int type)
{
- int type = flock_translate_cmd(cmd);
-
- if (type < 0)
- return ERR_PTR(type);
-
- if (fl == NULL) {
- fl = locks_alloc_lock();
- if (fl == NULL)
- return ERR_PTR(-ENOMEM);
- } else {
- locks_init_lock(fl);
- }
+ locks_init_lock(fl);
fl->fl_file = filp;
fl->fl_owner = filp;
@@ -447,8 +435,6 @@ flock_make_lock(struct file *filp, unsigned int cmd, struct file_lock *fl)
fl->fl_flags = FL_FLOCK;
fl->fl_type = type;
fl->fl_end = OFFSET_MAX;
-
- return fl;
}
static int assign_type(struct file_lock *fl, long type)
@@ -2097,10 +2083,9 @@ EXPORT_SYMBOL(locks_lock_inode_wait);
*/
SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
{
+ int can_sleep, error, unlock, type;
struct fd f = fdget(fd);
- struct file_lock *lock;
- int can_sleep, unlock;
- int error;
+ struct file_lock fl;
error = -EBADF;
if (!f.file)
@@ -2127,28 +2112,27 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
goto out_putf;
}
- lock = flock_make_lock(f.file, cmd, NULL);
- if (IS_ERR(lock)) {
- error = PTR_ERR(lock);
+ type = flock_translate_cmd(cmd);
+ if (type < 0) {
+ error = type;
goto out_putf;
}
+ flock_make_lock(f.file, &fl, type);
+
if (can_sleep)
- lock->fl_flags |= FL_SLEEP;
+ fl.fl_flags |= FL_SLEEP;
- error = security_file_lock(f.file, lock->fl_type);
+ error = security_file_lock(f.file, fl.fl_type);
if (error)
- goto out_free;
+ goto out_putf;
if (f.file->f_op->flock)
error = f.file->f_op->flock(f.file,
(can_sleep) ? F_SETLKW : F_SETLK,
- lock);
+ &fl);
else
- error = locks_lock_file_wait(f.file, lock);
-
- out_free:
- locks_free_lock(lock);
+ error = locks_lock_file_wait(f.file, &fl);
out_putf:
fdput(f);
@@ -2614,7 +2598,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
if (list_empty(&flctx->flc_flock))
return;
- flock_make_lock(filp, LOCK_UN, &fl);
+ flock_make_lock(filp, &fl, F_UNLCK);
fl.fl_flags |= FL_CLOSE;
if (filp->f_op->flock)
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 2/2] fs/lock: Rearrange ops in flock syscall.
2022-07-17 4:35 [PATCH v3 0/2] fs/lock: Cleanup around flock syscall Kuniyuki Iwashima
2022-07-17 4:35 ` [PATCH v3 1/2] fs/lock: Don't allocate file_lock in flock_make_lock() Kuniyuki Iwashima
@ 2022-07-17 4:35 ` Kuniyuki Iwashima
2022-07-18 10:43 ` [PATCH v3 0/2] fs/lock: Cleanup around " Jeff Layton
2 siblings, 0 replies; 5+ messages in thread
From: Kuniyuki Iwashima @ 2022-07-17 4:35 UTC (permalink / raw
To: Jeff Layton, Chuck Lever, Alexander Viro
Cc: Kuniyuki Iwashima, Kuniyuki Iwashima, linux-fsdevel
The previous patch added flock_translate_cmd() in flock syscall.
The test and the other one for LOCK_MAND do not depend on struct
fd and are cheaper, so we can put them at the top and defer
fdget() after that.
Also, we can remove the unlock variable and use type instead.
While at it, we fix this checkpatch error.
CHECK: spaces preferred around that '|' (ctx:VxV)
#45: FILE: fs/locks.c:2099:
+ if (type != F_UNLCK && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
^
Finally, we can move the can_sleep part just before we use it.
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
fs/locks.c | 43 +++++++++++++++++++------------------------
1 file changed, 19 insertions(+), 24 deletions(-)
diff --git a/fs/locks.c b/fs/locks.c
index b134eaefd7d6..c266cfdc3291 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2083,20 +2083,9 @@ EXPORT_SYMBOL(locks_lock_inode_wait);
*/
SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
{
- int can_sleep, error, unlock, type;
- struct fd f = fdget(fd);
+ int can_sleep, error, type;
struct file_lock fl;
-
- error = -EBADF;
- if (!f.file)
- goto out;
-
- can_sleep = !(cmd & LOCK_NB);
- cmd &= ~LOCK_NB;
- unlock = (cmd == LOCK_UN);
-
- if (!unlock && !(f.file->f_mode & (FMODE_READ|FMODE_WRITE)))
- goto out_putf;
+ struct fd f;
/*
* LOCK_MAND locks were broken for a long time in that they never
@@ -2108,35 +2097,41 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
*/
if (cmd & LOCK_MAND) {
pr_warn_once("Attempt to set a LOCK_MAND lock via flock(2). This support has been removed and the request ignored.\n");
- error = 0;
- goto out_putf;
+ return 0;
}
- type = flock_translate_cmd(cmd);
- if (type < 0) {
- error = type;
+ type = flock_translate_cmd(cmd & ~LOCK_NB);
+ if (type < 0)
+ return type;
+
+ error = -EBADF;
+ f = fdget(fd);
+ if (!f.file)
+ return error;
+
+ if (type != F_UNLCK && !(f.file->f_mode & (FMODE_READ | FMODE_WRITE)))
goto out_putf;
- }
flock_make_lock(f.file, &fl, type);
- if (can_sleep)
- fl.fl_flags |= FL_SLEEP;
-
error = security_file_lock(f.file, fl.fl_type);
if (error)
goto out_putf;
+ can_sleep = !(cmd & LOCK_NB);
+ if (can_sleep)
+ fl.fl_flags |= FL_SLEEP;
+
if (f.file->f_op->flock)
error = f.file->f_op->flock(f.file,
- (can_sleep) ? F_SETLKW : F_SETLK,
+ (can_sleep) ? F_SETLKW : F_SETLK,
&fl);
else
error = locks_lock_file_wait(f.file, &fl);
out_putf:
fdput(f);
- out:
+
return error;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 0/2] fs/lock: Cleanup around flock syscall.
2022-07-17 4:35 [PATCH v3 0/2] fs/lock: Cleanup around flock syscall Kuniyuki Iwashima
2022-07-17 4:35 ` [PATCH v3 1/2] fs/lock: Don't allocate file_lock in flock_make_lock() Kuniyuki Iwashima
2022-07-17 4:35 ` [PATCH v3 2/2] fs/lock: Rearrange ops in flock syscall Kuniyuki Iwashima
@ 2022-07-18 10:43 ` Jeff Layton
2 siblings, 0 replies; 5+ messages in thread
From: Jeff Layton @ 2022-07-18 10:43 UTC (permalink / raw
To: Kuniyuki Iwashima, Chuck Lever, Alexander Viro
Cc: Kuniyuki Iwashima, linux-fsdevel
On Sat, 2022-07-16 at 21:35 -0700, Kuniyuki Iwashima wrote:
> The first patch removes allocate-and-free for struct file_lock
> in flock syscall and the second patch rearrange some operations.
>
> Changes:
> v3:
> * Test LOCK_MAND first in patch 2
>
> v2: https://lore.kernel.org/linux-fsdevel/20220716233343.22106-1-kuniyu@amazon.com/
> * Use F_UNLCK in locks_remove_flock() (Chuck Lever)
> * Fix uninitialised error in flock syscall (kernel test robot)
> * Fix error when setting LOCK_NB
> * Split patches not to mix different kinds of optimisations and
> not to miss errors reported by kernel test robot
>
> v1: https://lore.kernel.org/linux-fsdevel/20220716013140.61445-1-kuniyu@amazon.com/
>
>
> Kuniyuki Iwashima (2):
> fs/lock: Don't allocate file_lock in flock_make_lock().
> fs/lock: Rearrange ops in flock syscall.
>
> fs/locks.c | 77 ++++++++++++++++++++----------------------------------
> 1 file changed, 28 insertions(+), 49 deletions(-)
>
This looks good, and I've added it to the branch I feed into linux-next.
If all goes well, I'll plan to ask Linus to pull this in for v5.20.
Since you respun it, I dropped Chuck's Reviewed-by. Chuck, please re-
review if you want me to put that back.
Thanks!
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/2] fs/lock: Don't allocate file_lock in flock_make_lock().
2022-07-17 4:35 ` [PATCH v3 1/2] fs/lock: Don't allocate file_lock in flock_make_lock() Kuniyuki Iwashima
@ 2022-07-18 13:54 ` Chuck Lever III
0 siblings, 0 replies; 5+ messages in thread
From: Chuck Lever III @ 2022-07-18 13:54 UTC (permalink / raw
To: Jeff Layton; +Cc: Al Viro, Kuniyuki Iwashima, Kuniyuki Iwashima, linux-fsdevel
> On Jul 17, 2022, at 12:35 AM, Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Two functions, flock syscall and locks_remove_flock(), call
> flock_make_lock(). It allocates struct file_lock from slab
> cache if its argument fl is NULL.
>
> When we call flock syscall, we pass NULL to allocate memory
> for struct file_lock. However, we always free it at the end
> by locks_free_lock(). We need not allocate it and instead
> should use a local variable as locks_remove_flock() does.
>
> Also, the validation for flock_translate_cmd() is not necessary
> for locks_remove_flock(). So we move the part to flock syscall
> and make flock_make_lock() return nothing.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
v3 Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> fs/locks.c | 46 +++++++++++++++-------------------------------
> 1 file changed, 15 insertions(+), 31 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index ca28e0e50e56..b134eaefd7d6 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -425,21 +425,9 @@ static inline int flock_translate_cmd(int cmd) {
> }
>
> /* Fill in a file_lock structure with an appropriate FLOCK lock. */
> -static struct file_lock *
> -flock_make_lock(struct file *filp, unsigned int cmd, struct file_lock *fl)
> +static void flock_make_lock(struct file *filp, struct file_lock *fl, int type)
> {
> - int type = flock_translate_cmd(cmd);
> -
> - if (type < 0)
> - return ERR_PTR(type);
> -
> - if (fl == NULL) {
> - fl = locks_alloc_lock();
> - if (fl == NULL)
> - return ERR_PTR(-ENOMEM);
> - } else {
> - locks_init_lock(fl);
> - }
> + locks_init_lock(fl);
>
> fl->fl_file = filp;
> fl->fl_owner = filp;
> @@ -447,8 +435,6 @@ flock_make_lock(struct file *filp, unsigned int cmd, struct file_lock *fl)
> fl->fl_flags = FL_FLOCK;
> fl->fl_type = type;
> fl->fl_end = OFFSET_MAX;
> -
> - return fl;
> }
>
> static int assign_type(struct file_lock *fl, long type)
> @@ -2097,10 +2083,9 @@ EXPORT_SYMBOL(locks_lock_inode_wait);
> */
> SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
> {
> + int can_sleep, error, unlock, type;
> struct fd f = fdget(fd);
> - struct file_lock *lock;
> - int can_sleep, unlock;
> - int error;
> + struct file_lock fl;
>
> error = -EBADF;
> if (!f.file)
> @@ -2127,28 +2112,27 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
> goto out_putf;
> }
>
> - lock = flock_make_lock(f.file, cmd, NULL);
> - if (IS_ERR(lock)) {
> - error = PTR_ERR(lock);
> + type = flock_translate_cmd(cmd);
> + if (type < 0) {
> + error = type;
> goto out_putf;
> }
>
> + flock_make_lock(f.file, &fl, type);
> +
> if (can_sleep)
> - lock->fl_flags |= FL_SLEEP;
> + fl.fl_flags |= FL_SLEEP;
>
> - error = security_file_lock(f.file, lock->fl_type);
> + error = security_file_lock(f.file, fl.fl_type);
> if (error)
> - goto out_free;
> + goto out_putf;
>
> if (f.file->f_op->flock)
> error = f.file->f_op->flock(f.file,
> (can_sleep) ? F_SETLKW : F_SETLK,
> - lock);
> + &fl);
> else
> - error = locks_lock_file_wait(f.file, lock);
> -
> - out_free:
> - locks_free_lock(lock);
> + error = locks_lock_file_wait(f.file, &fl);
>
> out_putf:
> fdput(f);
> @@ -2614,7 +2598,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
> if (list_empty(&flctx->flc_flock))
> return;
>
> - flock_make_lock(filp, LOCK_UN, &fl);
> + flock_make_lock(filp, &fl, F_UNLCK);
> fl.fl_flags |= FL_CLOSE;
>
> if (filp->f_op->flock)
> --
> 2.30.2
>
--
Chuck Lever
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-18 13:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-17 4:35 [PATCH v3 0/2] fs/lock: Cleanup around flock syscall Kuniyuki Iwashima
2022-07-17 4:35 ` [PATCH v3 1/2] fs/lock: Don't allocate file_lock in flock_make_lock() Kuniyuki Iwashima
2022-07-18 13:54 ` Chuck Lever III
2022-07-17 4:35 ` [PATCH v3 2/2] fs/lock: Rearrange ops in flock syscall Kuniyuki Iwashima
2022-07-18 10:43 ` [PATCH v3 0/2] fs/lock: Cleanup around " Jeff Layton
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).