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)
