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)
