Hi Andy
> -----Original Message-----
> From: Justin He
> Sent: Thursday, June 24, 2021 1:49 PM
> To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Cc: Petr Mladek <pmladek@xxxxxxxx>; Steven Rostedt <rostedt@xxxxxxxxxxx>;
> Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>; 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-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> fsdevel@xxxxxxxxxxxxxxx; Matthew Wilcox <willy@xxxxxxxxxxxxx>; Christoph
> Hellwig <hch@xxxxxxxxxxxxx>; nd <nd@xxxxxxx>
> Subject: RE: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()
>
> Hi Andy
>
> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Sent: Wednesday, June 23, 2021 5:11 PM
> > To: Justin He <Justin.He@xxxxxxx>
> > Cc: Petr Mladek <pmladek@xxxxxxxx>; Steven Rostedt <rostedt@xxxxxxxxxxx>;
> > Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>; 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-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > fsdevel@xxxxxxxxxxxxxxx; Matthew Wilcox <willy@xxxxxxxxxxxxx>; Christoph
> > Hellwig <hch@xxxxxxxxxxxxx>; nd <nd@xxxxxxx>
> > 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)
