Messages in this thread | | | From | Justin He <> | Subject | RE: [PATCH v2 1/4] fs: introduce helper d_path_unsafe() | Date | Wed, 14 Jul 2021 08:33:10 +0000 |
| |
Hi Andy
> -----Original Message----- > From: Justin He > Sent: Thursday, June 24, 2021 1:49 PM > To: Andy Shevchenko <andriy.shevchenko@linux.intel.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() > > 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()?
This smp_load_acquire() is to add a barrier btw ->name and ->len. This is the pair of smp_store_release() in __d_alloc(). Please see the details in commit 7088efa9137a15d7d21e3abce73e40c9c8a18d68
fs/dcache: Use release-acquire for name/length update
The code in __d_alloc() carefully orders filling in the NUL character of the name (and the length, hash, and the name itself) with assigning of the name itself. However, prepend_name() does not order the accesses to the ->name and ->len fields, other than on TSO systems. This commit therefore replaces prepend_name()'s READ_ONCE() of ->name with an smp_load_acquire(), which orders against the subsequent READ_ONCE() of ->len. Because READ_ONCE() now incorporates smp_read_barrier_depends(), prepend_name()'s smp_read_barrier_depends() is removed. Finally, to save a line, the smp_wmb()/store pair in __d_alloc() is replaced by smp_store_release().
I prefer to keep it as previous, what do you think of it?
-- Cheers, Justin (Jia He)
| |