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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
