RE: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()

[Justin He]

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)