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

Hi Andy

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Sent: Tuesday, June 22, 2021 10:37 PM
To: Justin He <redacted>
Cc: Petr Mladek <pmladek@suse.com>; Steven Rostedt <rostedt@goodmis.org>;
Sergey Senozhatsky [off-list ref]; Rasmus Villemoes
[off-list ref]; Jonathan Corbet [off-list ref]; Alexander
Viro [off-list ref]; Linus Torvalds <torvalds@linux-
foundation.org>; Peter Zijlstra (Intel) [off-list ref]; Eric
Biggers [off-list ref]; Ahmed S. Darwish [off-list ref];
linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
fsdevel@vger.kernel.org; Matthew Wilcox [off-list ref]; Christoph
Hellwig [off-list ref]; nd [off-list ref]
Subject: Re: [PATCH v5 1/4] fs: introduce helper d_path_unsafe()

On Tue, Jun 22, 2021 at 10:06:31PM +0800, Jia He wrote:

quoted

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

Missed period at the end of sentence.

Okay

quoted

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.

quoted

If someone invokes snprintf() with small but positive space,
prepend_name_with_len() moves and copies the string partially.

More than that, kasprintf() will pass NULL _buf_ and _end_ as the
parameters. Hence return at the very beginning with false in this case.

These two paragraphs are talking about printf() interface, while patch has
nothing to do with it. Please, rephrase in a way that it doesn't refer to
the
particular callers. Better to mention them in the corresponding printf()
patch(es).

Okay

...

quoted

  * prepend_name - prepend a pathname in front of current buffer pointer
- * @buffer: buffer pointer
- * @buflen: allocated length of the buffer
+ * @p: prepend buffer which contains buffer pointer and allocated length

quoted

  * @name:   name string and length qstr structure

Indentation issue btw, can be fixed in the same patch.

Okay

quoted

  *
  * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to

Shouldn't this be a separate change with corresponding Fixes tag?

Sorry, I don't quite understand here.
What do you want to fix?

...

quoted

+/**
+ * d_path_unsafe - return the full path of a dentry without taking
+ * any seqlock/spinlock. This helper is typical for debugging purposes.

Seems you ignored my comment, or forget to test, or compile test with
kernel
doc validator enabled doesn't show any issues. If it's the latter, we have
to
fix kernel doc validator.

TL;DR: describe parameters as well.

My bad. Apologize for that.

quoted

+ */

...

quoted

+	struct path root;
+	struct mount *mnt = real_mount(path->mnt);
+	DECLARE_BUFFER(b, buf, buflen);

Can wee keep this in reversed xmas tree order?

Sure 😊


--
Cheers,
Justin (Jia He)