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()? > > + 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)
