Messages in this thread | | | From | Justin He <> | Subject | RE: [PATCH v2 1/4] fs: introduce helper d_path_unsafe() | Date | Thu, 24 Jun 2021 05:48:32 +0000 |
| |
Hi Andy
> -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Wednesday, June 23, 2021 5:11 PM > To: Justin He <Justin.He@arm.com> > Cc: Petr Mladek <pmladek@suse.com>; Steven Rostedt <rostedt@goodmis.org>; > Sergey Senozhatsky <senozhatsky@chromium.org>; 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-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > fsdevel@vger.kernel.org; Matthew Wilcox <willy@infradead.org>; Christoph > Hellwig <hch@infradead.org>; nd <nd@arm.com> > Subject: Re: [PATCH v2 1/4] fs: introduce helper d_path_unsafe() > > On Wed, Jun 23, 2021 at 01:50:08PM +0800, Jia He wrote: > > 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. > > ... > > > /** > > * 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 > > This should be separate patch. You are sending new version too fast... > Instead of speeding up it will slow down the review process.
Okay, sorry about sending the new version too fast. I will slow it down and check carefully before sending out. > > ... > > > + const char *dname = smp_load_acquire(&name->name); /* ^^^ */ > > I have commented on the comment here. What does it mean for mere reader? >
Do you suggest making the comment "/* ^^^ */" more clear? It is detailed already in prepend_name_with_len()'s comments: > * Load acquire is needed to make sure that we see that terminating NUL, > * which is similar to prepend_name().
Or do you suggest removing the smp_load_acquire()?
> > + int dlen = READ_ONCE(name->len); > > + char *s; > > + int last_len = p->len; > > Reversed xmas tree order, please. > > The rule of thumb is when you have gotten a comment against a piece of code, > try to fix all similar places at once.
Sorry, I misunderstood it, will change it with reverse xmas tree order.
-- Cheers, Justin (Jia He)
| |