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

[Justin He]

Hi Andy
Thanks for the comments, I will resolve you mentioned typo/grammar issues

Some answer below

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Wednesday, June 16, 2021 4:41 AM
> To: Justin He <Justin.He@xxxxxxx>
> Cc: 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>; Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>; Eric
> Biggers <ebiggers@xxxxxxxxxx>; Ahmed S. Darwish <a.darwish@xxxxxxxxxxxxx>;
> Linux Documentation List <linux-doc@xxxxxxxxxxxxxxx>; Linux Kernel Mailing
> List <linux-kernel@xxxxxxxxxxxxxxx>; Linux FS Devel <linux-
> fsdevel@xxxxxxxxxxxxxxx>; Matthew Wilcox <willy@xxxxxxxxxxxxx>
> Subject: Re: [PATCH RFCv4 1/4] fs: introduce helper d_path_unsafe()
> 
> On Tue, Jun 15, 2021 at 6:56 PM Jia He <justin.he@xxxxxxx> wrote:
> >
> > This helper is similar to d_path except that it doesn't take any
> > seqlock/spinlock. It is typical for debugging purpose. Besides,
> 
> purposes
> 
> > an additional return value *prenpend_len* is used to get the full
> > path length of the dentry.
> >
> > prepend_name_with_len() enhances the behavior of prepend_name().
> > Previously it will skip the loop at once in __prepen_path() when the
> > space is not enough. __prepend_path() gets the full length of dentry
> > together with the parent recusively.
> 
> recursively
> 
> >
> > Besides, if someone invokes snprintf with small but positive space,
> > prepend_name_with() needs to move and copy the string partially.
> >
> > More than that, kasnprintf will pass NULL _buf_ and _end_, hence
> 
> kasprintf()
> 
> > it returns at the very beginning with false in this case;
> >
> > Suggested-by: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > ---
> >  fs/d_path.c            | 83 +++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/dcache.h |  1 +
> >  2 files changed, 82 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/d_path.c b/fs/d_path.c
> > index 23a53f7b5c71..4fc224eadf58 100644
> > --- a/fs/d_path.c
> > +++ b/fs/d_path.c
> > @@ -68,9 +68,66 @@ static bool prepend_name(struct prepend_buffer *p,
> const struct qstr *name)
> >         return true;
> >  }
> >
> > +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); /* ^^^ */
> 
> What does this funny comment mean?

It is inherited from the prepend_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);
> > +
> > +               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) {
> > +                       /* This 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) {
> > +                       /* Partially filled, and drop last dentry name */
> > +                       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;
> 
> I'm wondering why can't memcpy() be used here as well.


From the comments of commit 7a5cf791:

>However, there may be mismatch between length and pointer.
> * The length cannot be trusted, we need to copy it byte-by-byte until
> * the length is reached or a null byte is found.

Seems we shouldn't use memcpy/strcpy here

> 
> > +       }
> > +       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 +154,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);
> >                 dentry = parent;
> >         }
> >         return 0;
> > @@ -263,6 +319,29 @@ char *d_path(const struct path *path, char *buf, int
> buflen)
> >  }
> >  EXPORT_SYMBOL(d_path);
> >
> > +/**
> > + * d_path_unsafe - fast return the full path of a dentry without taking
> > + * any seqlock/spinlock. This helper is typical for debugging purpose.
> 
> purposes
> 
> Haven't you got kernel doc validation warnings? Please, describe
> parameters as well.

I will check it and update if there is a warning, thanks for the reminder.


--
Cheers,
Justin (Jia He)