RE: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()

[Justin He]

Hi Petr

> -----Original Message-----
> From: Petr Mladek <pmladek@xxxxxxxx>
> Sent: Monday, June 28, 2021 5:07 PM
> To: Justin He <Justin.He@xxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Peter Zijlstra
> (Intel) <peterz@xxxxxxxxxxxxx>; Eric Biggers <ebiggers@xxxxxxxxxx>; Ahmed S.
> Darwish <a.darwish@xxxxxxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx; Matthew Wilcox
> <willy@xxxxxxxxxxxxx>; Christoph Hellwig <hch@xxxxxxxxxxxxx>; nd
> <nd@xxxxxxx>; Steven Rostedt <rostedt@xxxxxxxxxxx>; Sergey Senozhatsky
> <senozhatsky@xxxxxxxxxxxx>; Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>;
> Jonathan Corbet <corbet@xxxxxxx>; Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>;
> Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> 
> On Mon 2021-06-28 05:13:51, Justin He wrote:
> > Hi Andy, Petr
> >
> > > -----Original Message-----
> > > From: Jia He <justin.he@xxxxxxx>
> > > Sent: Wednesday, June 23, 2021 1:50 PM
> > > To: Petr Mladek <pmladek@xxxxxxxx>; Steven Rostedt
> <rostedt@xxxxxxxxxxx>;
> > > Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>; Andy Shevchenko
> > > <andriy.shevchenko@xxxxxxxxxxxxxxx>; Rasmus Villemoes
> > > <linux@xxxxxxxxxxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; Alexander
> > > Viro <viro@xxxxxxxxxxxxxxxxxx>; Linus Torvalds <torvalds@linux-
> > > foundation.org>
> > > Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>; Eric Biggers
> > > <ebiggers@xxxxxxxxxx>; Ahmed S. Darwish <a.darwish@xxxxxxxxxxxxx>;
> linux-
> > > doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > > fsdevel@xxxxxxxxxxxxxxx; Matthew Wilcox <willy@xxxxxxxxxxxxx>;
> Christoph
> > > Hellwig <hch@xxxxxxxxxxxxx>; nd <nd@xxxxxxx>; Justin He
> <Justin.He@xxxxxxx>
> > > Subject: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
> > >
> > > This helper is similar to d_path() except that it doesn't take any
> > > seqlock/spinlock. It is typical for debugging purposes. Besides,
> > > an additional return value *prenpend_len* is used to get the full
> > > path length of the dentry, ingoring the tail '\0'.
> > > the full path length = end - buf - prepend_length - 1.
> > >
> > > Previously it will skip the prepend_name() loop at once in
> > > __prepen_path() when the buffer length is not enough or even negative.
> > > prepend_name_with_len() will get the full length of dentry name
> > > together with the parent recursively regardless of the buffer length.
> > >
> > > Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > > ---
> > >  fs/d_path.c            | 122 ++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/dcache.h |   1 +
> > >  2 files changed, 116 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/d_path.c b/fs/d_path.c
> > > index 23a53f7b5c71..7a3ea88f8c5c 100644
> > > --- a/fs/d_path.c
> > > +++ b/fs/d_path.c
> > > @@ -33,9 +33,8 @@ static void prepend(struct prepend_buffer *p, const
> char
> > > *str, int namelen)
> > >
> > >  /**
> > >   * prepend_name - prepend a pathname in front of current buffer
> pointer
> > > - * @buffer: buffer pointer
> > > - * @buflen: allocated length of the buffer
> > > - * @name:   name string and length qstr structure
> > > + * @p: prepend buffer which contains buffer pointer and allocated
> length
> > > + * @name: name string and length qstr structure
> > >   *
> > >   * With RCU path tracing, it may race with d_move(). Use READ_ONCE()
> to
> > >   * make sure that either the old or the new name pointer and length
> are
> > > @@ -68,9 +67,84 @@ static bool prepend_name(struct prepend_buffer *p,
> > > const struct qstr *name)
> > >  	return true;
> > >  }
> > >
> > > +/**
> > > + * prepend_name_with_len - prepend a pathname in front of current
> buffer
> > > + * pointer with limited orig_buflen.
> > > + * @p: prepend buffer which contains buffer pointer and allocated
> length
> > > + * @name: name string and length qstr structure
> > > + * @orig_buflen: original length of the buffer
> > > + *
> > > + * p.ptr is updated each time when prepends dentry name and its parent.
> > > + * Given the orginal buffer length might be less than name string, the
> > > + * dentry name can be moved or truncated. Returns at once if !buf or
> > > + * original length is not positive to avoid memory copy.
> > > + *
> > > + * Load acquire is needed to make sure that we see that terminating
> NUL,
> > > + * which is similar to prepend_name().
> > > + */
> > > +static bool prepend_name_with_len(struct prepend_buffer *p,
> > > +				  const struct qstr *name, int orig_buflen)
> > > +{
> > > +	const char *dname = smp_load_acquire(&name->name); /* ^^^ */
> > > +	int dlen = READ_ONCE(name->len);
> > > +	char *s;
> > > +	int last_len = p->len;
> > > +
> > > +	p->len -= dlen + 1;
> > > +
> > > +	if (unlikely(!p->buf))
> > > +		return false;
> > > +
> > > +	if (orig_buflen <= 0)
> > > +		return false;
> > > +
> > > +	/*
> > > +	 * The first time we overflow the buffer. Then fill the string
> > > +	 * partially from the beginning
> > > +	 */
> > > +	if (unlikely(p->len < 0)) {
> > > +		int buflen = strlen(p->buf);
> > > +
> > > +		/* memcpy src */
> > > +		s = p->buf;
> > > +
> > > +		/* Still have small space to fill partially */
> > > +		if (last_len > 0) {
> > > +			p->buf -= last_len;
> > > +			buflen += last_len;
> > > +		}
> > > +
> > > +		if (buflen > dlen + 1) {
> > > +			/* Dentry name can be fully filled */
> > > +			memmove(p->buf + dlen + 1, s, buflen - dlen - 1);
> > > +			p->buf[0] = '/';
> > > +			memcpy(p->buf + 1, dname, dlen);
> > > +		} else if (buflen > 0) {
> > > +			/* Can be partially filled, and drop last dentry */
> > > +			p->buf[0] = '/';
> > > +			memcpy(p->buf + 1, dname, buflen - 1);
> > > +		}
> > > +
> > > +		return false;
> > > +	}
> > > +
> > > +	s = p->buf -= dlen + 1;
> > > +	*s++ = '/';
> > > +	while (dlen--) {
> > > +		char c = *dname++;
> > > +
> > > +		if (!c)
> > > +			break;
> > > +		*s++ = c;
> > > +	}
> > > +	return true;
> > > +}
> > > +
> > >  static int __prepend_path(const struct dentry *dentry, const struct
> mount
> > > *mnt,
> > >  			  const struct path *root, struct prepend_buffer *p)
> > >  {
> > > +	int orig_buflen = p->len;
> > > +
> > >  	while (dentry != root->dentry || &mnt->mnt != root->mnt) {
> > >  		const struct dentry *parent = READ_ONCE(dentry->d_parent);
> > >
> > > @@ -97,8 +171,7 @@ static int __prepend_path(const struct dentry
> *dentry,
> > > const struct mount *mnt,
> > >  			return 3;
> > >
> > >  		prefetch(parent);
> > > -		if (!prepend_name(p, &dentry->d_name))
> > > -			break;
> > > +		prepend_name_with_len(p, &dentry->d_name, orig_buflen);
> >
> > I have new concern here.
> > Previously,  __prepend_path() would break the loop at once when p.len<0.
> > And the return value of __prepend_path() was 0.
> > The caller of prepend_path() would typically check as follows:
> >   if (prepend_path(...) > 0)
> >   	do_sth();
> >
> > After I replaced prepend_name() with prepend_name_with_len(),
> > the return value of prepend_path() is possibly positive
> > together with p.len<0. The behavior is different from previous.
> 
> I do not feel qualified to make decision here.I do not have enough
> experience with this code.
> 
> Anyway, the new behavior looks correct to me. The return values
> 1, 2, 3 mean that there was something wrong with the path. The
> new code checks the entire path which looks correct to me.

Okay, got it. Thanks for the explanation.
Seems my concern is not necessary. I once compared the old and new
prepend_path return value, they are always the same in my test scenarios.

--
Cheers,
Justin (Jia He)