Re: [PATCH v3] fs/locks: print full locks information

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2021-03-11 at 11:45 +0800, Luo Longjun wrote:
> 在 2021/3/9 21:37, Jeff Layton 写道:
> > On Thu, 2021-02-25 at 22:58 -0500, Luo Longjun wrote:
> > > Commit fd7732e033e3 ("fs/locks: create a tree of dependent requests.")
> > > has put blocked locks into a tree.
> > > 
> > > So, with a for loop, we can't check all locks information.
> > > 
> > > To solve this problem, we should traverse the tree.
> > > 
> > > Signed-off-by: Luo Longjun <luolongjun@huawei.com>
> > > ---
> > >   fs/locks.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++--------
> > >   1 file changed, 56 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index 99ca97e81b7a..ecaecd1f1b58 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -2828,7 +2828,7 @@ struct locks_iterator {
> > >   };
> > >   
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >   static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> > > -			    loff_t id, char *pfx)
> > > +			    loff_t id, char *pfx, int repeat)
> > >   {
> > >   	struct inode *inode = NULL;
> > >   	unsigned int fl_pid;
> > > @@ -2844,7 +2844,11 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> > >   	if (fl->fl_file != NULL)
> > >   		inode = locks_inode(fl->fl_file);
> > >   
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > -	seq_printf(f, "%lld:%s ", id, pfx);
> > > +	seq_printf(f, "%lld: ", id);
> > > +
> > > +	if (repeat)
> > > +		seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx);
> > Shouldn't that be "%.*s" ?
> > 
> > Also, isn't this likely to end up walking past the end of "pfx" (or even
> > ending up at an address before the buffer)? You have this below:
> > 
> >      lock_get_status(f, fl, *id, "", 0);
> > 
> > ...so the "length" value you're passing into the format there is going
> > to be -1. It also seems like if you get a large "level" value in
> > locks_show, then you'll end up with a length that is much longer than
> > the actual string.
> 
> In my understanding, the difference of "%*s" and "%.*s" is that, "%*s" 
> specifies the minimal filed width while "%.*s" specifies the precision 
> of the string.
> 

Oh, right. I always forget about the first usage.

> Here, I use "%*s", because I want to print locks information in the 
> follwing format:
> 
> 2: FLOCK  ADVISORY  WRITE 110 00:02:493 0 EOF
> 2: -> FLOCK  ADVISORY  WRITE 111 00:02:493 0 EOF
> 2:  -> FLOCK  ADVISORY  WRITE 112 00:02:493 0 EOF
> 2:   -> FLOCK  ADVISORY  WRITE 113 00:02:493 0 EOF
> 2:    -> FLOCK  ADVISORY  WRITE 114 00:02:493 0 EOF
> 
> And also, there is another way to show there information, in the format 
> like:
> 
> 60: FLOCK  ADVISORY  WRITE 23350 08:02:4456514 0 EOF
> 60: -> FLOCK  ADVISORY  WRITE 23356 08:02:4456514 0 EOF
> 60: -> FLOCK  ADVISORY  WRITE 24217 08:02:4456514 0 EOF
> 60: -> FLOCK  ADVISORY  WRITE 24239 08:02:4456514 0 EOF
> 
> I think both formats are acceptable, but the first format shows 
> competition relationships between these locks.
> 

We might as well go with the one this patch implements. I like seeing
the chain of waiters as well, and it doesn't seem to break lslocks
(which is, to my knowledge, the only real programmatic consumer of this
file).

> In the following code:
> 
> > lock_get_status(f, fl, *id, "", 0);
> 
> repeat is 0, and in the function:
> 
> + if (repeat)
> +		seq_printf(f, "%*s", repeat - 1 + (int)strlen(pfx), pfx);
> 
> The if branch will not take effect, so it could not be -1.
> 


Good point.

Ok, I'll go ahead and put this one in linux-next for now. Assuming there
are no problems, it should make v5.13.

Thanks!
-- 
Jeff Layton <jlayton@kernel.org>