lkml.org 
[lkml]   [2021]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3] fs/locks: print full locks information
From
Date
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>

\
 
 \ /
  Last update: 2021-03-11 14:53    [W:0.069 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site