| Message ID | 20240116-flsplit-v1-11-c9d0f4370a5d@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | filelock: split struct file_lock into file_lock and file_lease structs | expand |
On Wed, 17 Jan 2024, Jeff Layton wrote: > I couldn't get them to work properly as macros, so convert them > to static inlines instead (which is probably better for the type safety > anyway). > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/locks.c | 46 +++++++++++++++++++++++++++++++++------------- > 1 file changed, 33 insertions(+), 13 deletions(-) > > diff --git a/fs/locks.c b/fs/locks.c > index 770aaa5809ba..eddf4d767d5d 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -70,10 +70,26 @@ > > #include <linux/uaccess.h> > > -#define IS_POSIX(fl) (fl->fl_core.fl_flags & FL_POSIX) Used 3 times... once as if (IS_POSIX(blocker) && !IS_OFDLCK(blocker)) Can an IS_POSIX lock also be IS_OFDLCK ?? > -#define IS_FLOCK(fl) (fl->fl_core.fl_flags & FL_FLOCK) Used once. > -#define IS_LEASE(fl) (fl->fl_core.fl_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT)) Used twice. Either "IS_LEASE" approves things that aren't leases, or FL_LEASE is not set on all leases.... Names could be improved. > -#define IS_OFDLCK(fl) (fl->fl_core.fl_flags & FL_OFDLCK) used 4 times - a clear winner :-) If it would me, I would simply discard these macros and open-code the tests. I don't think IS_FLOCK() is easier to read for someone who knows the code, and I think IS_LEASE() is actually harder to read for someone who doesn't know the code, as that it does it not really obvious. But this is just a suggestion, I won't push it. Thanks, NeilBrown > +static inline bool IS_POSIX(struct file_lock_core *flc) > +{ > + return flc->fl_flags & FL_POSIX; > +} > + > +static inline bool IS_FLOCK(struct file_lock_core *flc) > +{ > + return flc->fl_flags & FL_FLOCK; > +} > + > +static inline bool IS_OFDLCK(struct file_lock_core *flc) > +{ > + return flc->fl_flags & FL_OFDLCK; > +} > + > +static inline bool IS_LEASE(struct file_lock_core *flc) > +{ > + return flc->fl_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT); > +} > + > #define IS_REMOTELCK(fl) (fl->fl_core.fl_pid <= 0) > > static bool lease_breaking(struct file_lock *fl) > @@ -761,6 +777,7 @@ static void __locks_insert_block(struct file_lock *blocker, > struct file_lock *)) > { > struct file_lock *fl; > + struct file_lock_core *bflc; > BUG_ON(!list_empty(&waiter->fl_core.fl_blocked_member)); > > new_blocker: > @@ -773,7 +790,9 @@ static void __locks_insert_block(struct file_lock *blocker, > waiter->fl_core.fl_blocker = blocker; > list_add_tail(&waiter->fl_core.fl_blocked_member, > &blocker->fl_core.fl_blocked_requests); > - if (IS_POSIX(blocker) && !IS_OFDLCK(blocker)) > + > + bflc = &blocker->fl_core; > + if (IS_POSIX(bflc) && !IS_OFDLCK(bflc)) > locks_insert_global_blocked(&waiter->fl_core); > > /* The requests in waiter->fl_blocked are known to conflict with > @@ -998,6 +1017,7 @@ static int posix_locks_deadlock(struct file_lock *caller_fl, > struct file_lock *block_fl) > { > int i = 0; > + struct file_lock_core *flc = &caller_fl->fl_core; > > lockdep_assert_held(&blocked_lock_lock); > > @@ -1005,7 +1025,7 @@ static int posix_locks_deadlock(struct file_lock *caller_fl, > * This deadlock detector can't reasonably detect deadlocks with > * FL_OFDLCK locks, since they aren't owned by a process, per-se. > */ > - if (IS_OFDLCK(caller_fl)) > + if (IS_OFDLCK(flc)) > return 0; > > while ((block_fl = what_owner_is_waiting_for(block_fl))) { > @@ -2157,7 +2177,7 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns) > pid_t vnr; > struct pid *pid; > > - if (IS_OFDLCK(fl)) > + if (IS_OFDLCK(&fl->fl_core)) > return -1; > if (IS_REMOTELCK(fl)) > return fl->fl_core.fl_pid; > @@ -2721,19 +2741,19 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > if (repeat) > seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx); > > - if (IS_POSIX(fl)) { > + if (IS_POSIX(&fl->fl_core)) { > if (fl->fl_core.fl_flags & FL_ACCESS) > seq_puts(f, "ACCESS"); > - else if (IS_OFDLCK(fl)) > + else if (IS_OFDLCK(&fl->fl_core)) > seq_puts(f, "OFDLCK"); > else > seq_puts(f, "POSIX "); > > seq_printf(f, " %s ", > (inode == NULL) ? "*NOINODE*" : "ADVISORY "); > - } else if (IS_FLOCK(fl)) { > + } else if (IS_FLOCK(&fl->fl_core)) { > seq_puts(f, "FLOCK ADVISORY "); > - } else if (IS_LEASE(fl)) { > + } else if (IS_LEASE(&fl->fl_core)) { > if (fl->fl_core.fl_flags & FL_DELEG) > seq_puts(f, "DELEG "); > else > @@ -2748,7 +2768,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > } else { > seq_puts(f, "UNKNOWN UNKNOWN "); > } > - type = IS_LEASE(fl) ? target_leasetype(fl) : fl->fl_core.fl_type; > + type = IS_LEASE(&fl->fl_core) ? target_leasetype(fl) : fl->fl_core.fl_type; > > seq_printf(f, "%s ", (type == F_WRLCK) ? "WRITE" : > (type == F_RDLCK) ? "READ" : "UNLCK"); > @@ -2760,7 +2780,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > } else { > seq_printf(f, "%d <none>:0 ", fl_pid); > } > - if (IS_POSIX(fl)) { > + if (IS_POSIX(&fl->fl_core)) { > if (fl->fl_end == OFFSET_MAX) > seq_printf(f, "%Ld EOF\n", fl->fl_start); > else > > -- > 2.43.0 > >
On Wed, 2024-01-17 at 09:16 +1100, NeilBrown wrote: > On Wed, 17 Jan 2024, Jeff Layton wrote: > > I couldn't get them to work properly as macros, so convert them > > to static inlines instead (which is probably better for the type safety > > anyway). > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/locks.c | 46 +++++++++++++++++++++++++++++++++------------- > > 1 file changed, 33 insertions(+), 13 deletions(-) > > > > diff --git a/fs/locks.c b/fs/locks.c > > index 770aaa5809ba..eddf4d767d5d 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -70,10 +70,26 @@ > > > > #include <linux/uaccess.h> > > > > -#define IS_POSIX(fl) (fl->fl_core.fl_flags & FL_POSIX) > Used 3 times... once as > if (IS_POSIX(blocker) && !IS_OFDLCK(blocker)) > Can an IS_POSIX lock also be IS_OFDLCK ?? > Yes. They conflict with one another so they're both considered POSIX locks. > > > -#define IS_FLOCK(fl) (fl->fl_core.fl_flags & FL_FLOCK) > Used once. > > > -#define IS_LEASE(fl) (fl->fl_core.fl_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT)) > Used twice. Either "IS_LEASE" approves things that aren't leases, or > FL_LEASE is not set on all leases.... Names could be improved. > Good point. > > -#define IS_OFDLCK(fl) (fl->fl_core.fl_flags & FL_OFDLCK) > > used 4 times - a clear winner :-) > > If it would me, I would simply discard these macros and open-code the > tests. I don't think IS_FLOCK() is easier to read for someone who knows > the code, and I think IS_LEASE() is actually harder to read for someone > who doesn't know the code, as that it does it not really obvious. > > But this is just a suggestion, I won't push it. > > Thanks, > NeilBrown > > It's a good suggestion, and I considered doing this when I converted over the macros to inlines. I may go ahead and make this change for v2. > > +static inline bool IS_POSIX(struct file_lock_core *flc) > > +{ > > + return flc->fl_flags & FL_POSIX; > > +} > > + > > +static inline bool IS_FLOCK(struct file_lock_core *flc) > > +{ > > + return flc->fl_flags & FL_FLOCK; > > +} > > + > > +static inline bool IS_OFDLCK(struct file_lock_core *flc) > > +{ > > + return flc->fl_flags & FL_OFDLCK; > > +} > > + > > +static inline bool IS_LEASE(struct file_lock_core *flc) > > +{ > > + return flc->fl_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT); > > +} > > + > > #define IS_REMOTELCK(fl) (fl->fl_core.fl_pid <= 0) > > > > static bool lease_breaking(struct file_lock *fl) > > @@ -761,6 +777,7 @@ static void __locks_insert_block(struct file_lock *blocker, > > struct file_lock *)) > > { > > struct file_lock *fl; > > + struct file_lock_core *bflc; > > BUG_ON(!list_empty(&waiter->fl_core.fl_blocked_member)); > > > > new_blocker: > > @@ -773,7 +790,9 @@ static void __locks_insert_block(struct file_lock *blocker, > > waiter->fl_core.fl_blocker = blocker; > > list_add_tail(&waiter->fl_core.fl_blocked_member, > > &blocker->fl_core.fl_blocked_requests); > > - if (IS_POSIX(blocker) && !IS_OFDLCK(blocker)) > > + > > + bflc = &blocker->fl_core; > > + if (IS_POSIX(bflc) && !IS_OFDLCK(bflc)) > > locks_insert_global_blocked(&waiter->fl_core); > > > > /* The requests in waiter->fl_blocked are known to conflict with > > @@ -998,6 +1017,7 @@ static int posix_locks_deadlock(struct file_lock *caller_fl, > > struct file_lock *block_fl) > > { > > int i = 0; > > + struct file_lock_core *flc = &caller_fl->fl_core; > > > > lockdep_assert_held(&blocked_lock_lock); > > > > @@ -1005,7 +1025,7 @@ static int posix_locks_deadlock(struct file_lock *caller_fl, > > * This deadlock detector can't reasonably detect deadlocks with > > * FL_OFDLCK locks, since they aren't owned by a process, per-se. > > */ > > - if (IS_OFDLCK(caller_fl)) > > + if (IS_OFDLCK(flc)) > > return 0; > > > > while ((block_fl = what_owner_is_waiting_for(block_fl))) { > > @@ -2157,7 +2177,7 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns) > > pid_t vnr; > > struct pid *pid; > > > > - if (IS_OFDLCK(fl)) > > + if (IS_OFDLCK(&fl->fl_core)) > > return -1; > > if (IS_REMOTELCK(fl)) > > return fl->fl_core.fl_pid; > > @@ -2721,19 +2741,19 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > > if (repeat) > > seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx); > > > > - if (IS_POSIX(fl)) { > > + if (IS_POSIX(&fl->fl_core)) { > > if (fl->fl_core.fl_flags & FL_ACCESS) > > seq_puts(f, "ACCESS"); > > - else if (IS_OFDLCK(fl)) > > + else if (IS_OFDLCK(&fl->fl_core)) > > seq_puts(f, "OFDLCK"); > > else > > seq_puts(f, "POSIX "); > > > > seq_printf(f, " %s ", > > (inode == NULL) ? "*NOINODE*" : "ADVISORY "); > > - } else if (IS_FLOCK(fl)) { > > + } else if (IS_FLOCK(&fl->fl_core)) { > > seq_puts(f, "FLOCK ADVISORY "); > > - } else if (IS_LEASE(fl)) { > > + } else if (IS_LEASE(&fl->fl_core)) { > > if (fl->fl_core.fl_flags & FL_DELEG) > > seq_puts(f, "DELEG "); > > else > > @@ -2748,7 +2768,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > > } else { > > seq_puts(f, "UNKNOWN UNKNOWN "); > > } > > - type = IS_LEASE(fl) ? target_leasetype(fl) : fl->fl_core.fl_type; > > + type = IS_LEASE(&fl->fl_core) ? target_leasetype(fl) : fl->fl_core.fl_type; > > > > seq_printf(f, "%s ", (type == F_WRLCK) ? "WRITE" : > > (type == F_RDLCK) ? "READ" : "UNLCK"); > > @@ -2760,7 +2780,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, > > } else { > > seq_printf(f, "%d <none>:0 ", fl_pid); > > } > > - if (IS_POSIX(fl)) { > > + if (IS_POSIX(&fl->fl_core)) { > > if (fl->fl_end == OFFSET_MAX) > > seq_printf(f, "%Ld EOF\n", fl->fl_start); > > else > > > > -- > > 2.43.0 > > > > >
diff --git a/fs/locks.c b/fs/locks.c index 770aaa5809ba..eddf4d767d5d 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -70,10 +70,26 @@ #include <linux/uaccess.h> -#define IS_POSIX(fl) (fl->fl_core.fl_flags & FL_POSIX) -#define IS_FLOCK(fl) (fl->fl_core.fl_flags & FL_FLOCK) -#define IS_LEASE(fl) (fl->fl_core.fl_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT)) -#define IS_OFDLCK(fl) (fl->fl_core.fl_flags & FL_OFDLCK) +static inline bool IS_POSIX(struct file_lock_core *flc) +{ + return flc->fl_flags & FL_POSIX; +} + +static inline bool IS_FLOCK(struct file_lock_core *flc) +{ + return flc->fl_flags & FL_FLOCK; +} + +static inline bool IS_OFDLCK(struct file_lock_core *flc) +{ + return flc->fl_flags & FL_OFDLCK; +} + +static inline bool IS_LEASE(struct file_lock_core *flc) +{ + return flc->fl_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT); +} + #define IS_REMOTELCK(fl) (fl->fl_core.fl_pid <= 0) static bool lease_breaking(struct file_lock *fl) @@ -761,6 +777,7 @@ static void __locks_insert_block(struct file_lock *blocker, struct file_lock *)) { struct file_lock *fl; + struct file_lock_core *bflc; BUG_ON(!list_empty(&waiter->fl_core.fl_blocked_member)); new_blocker: @@ -773,7 +790,9 @@ static void __locks_insert_block(struct file_lock *blocker, waiter->fl_core.fl_blocker = blocker; list_add_tail(&waiter->fl_core.fl_blocked_member, &blocker->fl_core.fl_blocked_requests); - if (IS_POSIX(blocker) && !IS_OFDLCK(blocker)) + + bflc = &blocker->fl_core; + if (IS_POSIX(bflc) && !IS_OFDLCK(bflc)) locks_insert_global_blocked(&waiter->fl_core); /* The requests in waiter->fl_blocked are known to conflict with @@ -998,6 +1017,7 @@ static int posix_locks_deadlock(struct file_lock *caller_fl, struct file_lock *block_fl) { int i = 0; + struct file_lock_core *flc = &caller_fl->fl_core; lockdep_assert_held(&blocked_lock_lock); @@ -1005,7 +1025,7 @@ static int posix_locks_deadlock(struct file_lock *caller_fl, * This deadlock detector can't reasonably detect deadlocks with * FL_OFDLCK locks, since they aren't owned by a process, per-se. */ - if (IS_OFDLCK(caller_fl)) + if (IS_OFDLCK(flc)) return 0; while ((block_fl = what_owner_is_waiting_for(block_fl))) { @@ -2157,7 +2177,7 @@ static pid_t locks_translate_pid(struct file_lock *fl, struct pid_namespace *ns) pid_t vnr; struct pid *pid; - if (IS_OFDLCK(fl)) + if (IS_OFDLCK(&fl->fl_core)) return -1; if (IS_REMOTELCK(fl)) return fl->fl_core.fl_pid; @@ -2721,19 +2741,19 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, if (repeat) seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx); - if (IS_POSIX(fl)) { + if (IS_POSIX(&fl->fl_core)) { if (fl->fl_core.fl_flags & FL_ACCESS) seq_puts(f, "ACCESS"); - else if (IS_OFDLCK(fl)) + else if (IS_OFDLCK(&fl->fl_core)) seq_puts(f, "OFDLCK"); else seq_puts(f, "POSIX "); seq_printf(f, " %s ", (inode == NULL) ? "*NOINODE*" : "ADVISORY "); - } else if (IS_FLOCK(fl)) { + } else if (IS_FLOCK(&fl->fl_core)) { seq_puts(f, "FLOCK ADVISORY "); - } else if (IS_LEASE(fl)) { + } else if (IS_LEASE(&fl->fl_core)) { if (fl->fl_core.fl_flags & FL_DELEG) seq_puts(f, "DELEG "); else @@ -2748,7 +2768,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, } else { seq_puts(f, "UNKNOWN UNKNOWN "); } - type = IS_LEASE(fl) ? target_leasetype(fl) : fl->fl_core.fl_type; + type = IS_LEASE(&fl->fl_core) ? target_leasetype(fl) : fl->fl_core.fl_type; seq_printf(f, "%s ", (type == F_WRLCK) ? "WRITE" : (type == F_RDLCK) ? "READ" : "UNLCK"); @@ -2760,7 +2780,7 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl, } else { seq_printf(f, "%d <none>:0 ", fl_pid); } - if (IS_POSIX(fl)) { + if (IS_POSIX(&fl->fl_core)) { if (fl->fl_end == OFFSET_MAX) seq_printf(f, "%Ld EOF\n", fl->fl_start); else
I couldn't get them to work properly as macros, so convert them to static inlines instead (which is probably better for the type safety anyway). Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/locks.c | 46 +++++++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 13 deletions(-)