Messages in this thread | | | From | Justin He <> | Subject | RE: [PATCH RFCv4 1/4] fs: introduce helper d_path_unsafe() | Date | Wed, 16 Jun 2021 00:54:15 +0000 |
| |
Hi Andy Thanks for the comments, I will resolve you mentioned typo/grammar issues
Some answer below
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Wednesday, June 16, 2021 4:41 AM > To: Justin He <Justin.He@arm.com> > Cc: Petr Mladek <pmladek@suse.com>; Steven Rostedt <rostedt@goodmis.org>; > Sergey Senozhatsky <senozhatsky@chromium.org>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; Rasmus Villemoes > <linux@rasmusvillemoes.dk>; Jonathan Corbet <corbet@lwn.net>; Alexander > Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux- > foundation.org>; Peter Zijlstra (Intel) <peterz@infradead.org>; Eric > Biggers <ebiggers@google.com>; Ahmed S. Darwish <a.darwish@linutronix.de>; > Linux Documentation List <linux-doc@vger.kernel.org>; Linux Kernel Mailing > List <linux-kernel@vger.kernel.org>; Linux FS Devel <linux- > fsdevel@vger.kernel.org>; Matthew Wilcox <willy@infradead.org> > 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@arm.com> 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@infradead.org> > > Signed-off-by: Jia He <justin.he@arm.com> > > --- > > 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)
| |