All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jia He <justin.he@arm.com>, Petr Mladek <pmladek@suse.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Jonathan Corbet <corbet@lwn.net>, Al Viro <viro@ftp.linux.org.uk>,
	Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Ira Weiny <ira.weiny@intel.com>,
	Eric Biggers <ebiggers@google.com>,
	"Ahmed S. Darwish" <a.darwish@linutronix.de>,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-s390 <linux-s390@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: [PATCHSET] d_path cleanups
Date: Wed, 19 May 2021 00:43:15 +0000	[thread overview]
Message-ID: <YKRfI29BBnC255Vp@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <CAHk-=wih_O+0xG4QbLw-3XJ71Yh43_SFm3gp9swj8knzXoceZQ@mail.gmail.com>

	First of all, apologies for delays (one of the disks on the main
devel/testing box has become an ex-parrot, with... interesting recovery).

	Here's what I've got for carve-up of cleanups.  This stuff lives
in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.d_path,
individual patches in followups.  14 commits, all changes are to fs/d_path.c,
total being +133/-191.  Moderately tested, seems to work here.  Review
and testing would be welcome...

	Part 1: trivial preliminary cleanup

1/14) d_path: "\0" is {0,0}, not {0}
	A bunch of places used "\0" as a literal for 1-element array consisting
of NULs.  That should've been "", obviously...

	Part 2: untangling __dentry_path()

2/14) d_path: saner calling conventions for __dentry_path()
	__dentry_path() used to copy the calling conventions for dentry_path().
That was fine for use in dentry_path_raw(), but it created a lot of headache
in dentry_path(), since we might need a suffix (//deleted) in there, without
NUL between it and the pathname itself.  So we had to
	1) (possibly) put /deleted into buffer and remember where it went
	2) let __dentry_path() prepend NUL-terminated pathname
	3) override NUL with / if we had done (1)
Life becomes much easier if __dentry_path() does *NOT* put NUL in there.
Then dentry_path_raw() becomes 'put "" into buffer, then __dentry_path()'
and dentry_path() - 'put "" or "//deleted" into buffer, then __dentry_path()'.

Additionally, we switch the way buffer information is passed to one similar
to what we do in prepend()/prepend_name()/etc., i.e. pass the pointer to the
end of buffer instead of that to beginning.  That's what we'd been using
in __dentry_path() all along, and now that the callers are doing some prepend()
before calling __dentry_path(), they that value already on hand.

3/14)  d_path: regularize handling of root dentry in __dentry_path()
	All path-forming primitives boil down to sequence of prepend_name()
on dentries encountered along the way toward root.  Each time we prepend
/ + dentry name to the buffer.  Normally that does exactly what we want,
but there's a corner case when we don't call prepend_name() at all (in case
of __dentry_path() that happens if we are given root dentry).  We obviously
want to end up with "/", rather than "", so this corner case needs to be
handled.
	__dentry_path() used to manually put '/' in the end of buffer before
doing anything else, to be overwritten by the first call of prepend_name()
if one happens and to be left in place if we don't call prepend_name() at
all.  That required manually checking that we had space in the buffer
(prepend_name() and prepend() take care of such checks themselves) and lead
to clumsy keeping track of return value.
	A better approach is to check if the main loop has added anything
into the buffer and prepend "/" if it hasn't.  A side benefit of using prepend()
is that it does the right thing if we'd already run out of buffer, making
the overflow-handling logics simpler.

NB: the above might be worth putting into commit message.

	Part 3: overflow handling cleanups

We have an overcomplicated handling of overflows.  Primitives (prepend() and
prepend_name()) check if we'd run out of space and return -ENAMETOOLONG.
Then it's propagated all the way out by call chain.  However, the same
primitives are safe to call in case we'd *already* run out of space and
that condition is easily checked at any level of callchain.  The next
5 commits use that to simplify the control flow.

4/14)  d_path: get rid of path_with_deleted()
	expand into the sole caller, rearrange the suffix handing
along the lines of dentry_path().

5/14)  getcwd(2): saner logics around prepend_path() call
	Turn
		prepend_path()
		if it says it has run out of space, fail with ENAMETOOLONG
		if it wants "(unreachable) " prepended
			do so
			if that says it has run out of space, fail with ENAMETOOLONG
	into
		prepend_path()
		if it wants "(unreachable) " prepended
			do so
		if we see we'd run out of space at some point
			fail with ENAMETOOLONG

6/14)  d_path: don't bother with return value of prepend()
	Almost nothing is looking at return value of prepend() and it's
easy to get rid of the last stragglers...

7/14)  d_path: lift -ENAMETOOLONG handling into callers of prepend_path()
	It's easier to have prepend_path() return 0 on overflow and
check for overflow in the callers.  The logics is the same for all callers
(ran out of space => ERR_PTR(-ENAMETOOLONG)), so we get a bit of boilerplate,
but even with that the callers become simpler.  Added boilerplate will be
dealt with a couple of commits down the road.

8/14)  d_path: make prepend_name() boolean
	Unlike the case of prepend(), callers of prepend_name() really want
to see whether it has run out of space - the loops it's called in are
lockless and we could, in principle, end up spinning there for indetermined
amount of iterations.  Dropping out of loop if we run out of space in the
buffers serves as a backstop for (very unlikely) cases.
	However, all we care about is success/failure - we generate
ENAMETOOLONG in the callers, if not callers of callers, so we can bloody well
make it return bool.

	Part 4: introduction of prepend_buffer

9/14)  d_path: introduce struct prepend_buffer
	We've a lot of places where we have pairs of form (pointer to end
of buffer, amount of space left in front of that).  These sit in pairs of
variables located next to each other and usually passed by reference.
Turn those into instances of new type (struct prepend_buffer) and pass
reference to the pair instead of pairs of references to its fields.

Initialization (of form {buf + len, len}) turned into a macro (DECLARE_BUF),
to avoid brainos.  Extraction of string (buffer contents if we hadn't run
out of space, ERR_PTR(-ENAMETOOLONG) otherwise) is done by extract_string();
that eats the leftover boilerplate from earlier in the series.

	Part 5: extracting the lockless part of prepend_path()
The thing that started the entire mess had been an attempt to use d_path
machinery for vsprintf(); that can't grab rename_lock and mount_lock,
so we needed a variant of prepend_path() that would try to go without
those locks.  The obvious approach is to lift the internal loop of
prepend_path() into a new primitive.  However, that needs some massage
first to separate local variables - otherwise we end up with the
argument list from hell.

10/14) d_path: prepend_path(): get rid of vfsmnt
	redundant - we maintain mnt and vfsmnt through the loop,
with the latter being a pointer to mnt->mnt all along.
11/14) d_path: prepend_path(): lift resetting b in case when we'd return 3 out of loop
	the only place in the inner loop where we need p; we use it
to reset b in case we would return 3.  We can easily check that error is 3
after the loop and do resetting there.  Note that we only need that after
the *outer* loop - the body of that starts with assignment to b anyway.
12/14) d_path: prepend_path(): lift the inner loop into a new helper
	ta-da

	Part 6: followups

13/14) d_path: prepend_path() is unlikely to return non-zero
t14/14) getcwd(2): clean up error handling

  reply	other threads:[~2021-05-19  0:44 UTC|newest]

Thread overview: 75+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-08 12:25 [PATCH RFC 0/3] make '%pD' print full path for file Jia He
2021-05-08 12:25 ` [PATCH RFC 1/3] fs: introduce helper d_path_fast() Jia He
2021-05-08 15:30   ` Linus Torvalds
2021-05-08 19:13     ` Al Viro
2021-05-08 20:39       ` Linus Torvalds
2021-05-08 21:05         ` Al Viro
2021-05-08 22:17           ` Linus Torvalds
2021-05-08 22:46             ` Al Viro
2021-05-08 22:48               ` Linus Torvalds
2021-05-08 23:15               ` Al Viro
2021-05-08 23:18                 ` Al Viro
2021-05-09 22:58                 ` Eric W. Biederman
2021-05-10 12:51                   ` Christian Brauner
2021-05-10  7:20                 ` Christian Brauner
2021-05-08 22:42           ` Linus Torvalds
2021-05-08 22:47             ` Linus Torvalds
2021-05-09  2:28               ` Al Viro
2021-05-09  2:53                 ` Linus Torvalds
2021-05-19  0:43                   ` Al Viro [this message]
2021-05-19  0:48                     ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Al Viro
2021-05-19  0:48                       ` [PATCH 02/14] d_path: saner calling conventions for __dentry_path() Al Viro
2021-06-25  9:32                         ` Justin He
2021-07-07  4:52                           ` Justin He
2021-05-19  0:48                       ` [PATCH 03/14] d_path: regularize handling of root dentry in __dentry_path() Al Viro
2021-07-07  4:50                         ` Justin He
2021-05-19  0:48                       ` [PATCH 04/14] d_path: get rid of path_with_deleted() Al Viro
2021-05-19  0:48                       ` [PATCH 05/14] getcwd(2): saner logics around prepend_path() call Al Viro
2021-07-07  7:41                         ` Justin He
2021-05-19  0:48                       ` [PATCH 06/14] d_path: don't bother with return value of prepend() Al Viro
2021-06-24  6:13                         ` Justin He
2021-05-19  0:48                       ` [PATCH 07/14] d_path: lift -ENAMETOOLONG handling into callers of prepend_path() Al Viro
2021-06-25  9:18                         ` Justin He
2021-06-28  5:20                           ` Justin He
2021-05-19  0:48                       ` [PATCH 08/14] d_path: make prepend_name() boolean Al Viro
2021-05-20  9:12                         ` Justin He
2021-05-20  9:19                           ` Andy Shevchenko
2021-05-20 14:53                           ` Petr Mladek
2021-05-20 19:35                             ` Al Viro
2021-07-07  7:43                         ` Justin He
2021-05-19  0:48                       ` [PATCH 09/14] d_path: introduce struct prepend_buffer Al Viro
2021-06-23 13:28                         ` Justin He
2021-06-24  9:29                           ` Enrico Weigelt, metux IT consult
2021-06-25  0:43                             ` Justin He
2021-06-28 16:42                               ` Enrico Weigelt, metux IT consult
2021-06-28 17:10                                 ` Andy Shevchenko
2021-05-19  0:48                       ` [PATCH 10/14] d_path: prepend_path(): get rid of vfsmnt Al Viro
2021-05-19  0:48                       ` [PATCH 11/14] d_path: prepend_path(): lift resetting b in case when we'd return 3 out of loop Al Viro
2021-05-19  0:48                       ` [PATCH 12/14] d_path: prepend_path(): lift the inner loop into a new helper Al Viro
2021-05-19  8:07                         ` Andy Shevchenko
2021-05-19 15:55                           ` Al Viro
2021-07-07  7:52                         ` Justin He
2021-05-19  0:49                       ` [PATCH 13/14] d_path: prepend_path() is unlikely to return non-zero Al Viro
2021-06-25  8:00                         ` Justin He
2021-06-25 17:58                           ` Al Viro
2021-06-28  3:28                             ` Justin He
2021-06-28  4:14                               ` Al Viro
2021-06-28  4:36                                 ` Justin He
2021-06-28  4:37                         ` Justin He
2021-05-19  0:49                       ` [PATCH 14/14] getcwd(2): clean up error handling Al Viro
2021-07-07  8:03                         ` Justin He
2021-06-24  6:05                       ` [PATCH 01/14] d_path: "\0" is {0,0}, not {0} Justin He
2021-05-19  2:39                     ` [PATCHSET] d_path cleanups Linus Torvalds
2021-06-22 14:00                     ` Justin He
2021-05-09  2:20         ` [PATCH RFC 1/3] fs: introduce helper d_path_fast() Al Viro
2021-05-09  4:58           ` Al Viro
2021-05-10 16:16           ` Eric W. Biederman
2021-05-10 15:07         ` Justin He
2021-05-10 17:03           ` Linus Torvalds
2021-05-08 12:25 ` [PATCH RFC 2/3] lib/vsprintf.c: make %pD print full path for file Jia He
2021-05-10  3:46   ` Sergey Senozhatsky
2021-05-10 13:04   ` Petr Mladek
2021-05-10 14:25     ` Justin He
2021-05-27  7:20     ` Justin He
2021-05-27  9:14       ` Petr Mladek
2021-05-08 12:25 ` [PATCH RFC 3/3] s390/hmcdrv: remove the redundant directory path in debug message Jia He

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YKRfI29BBnC255Vp@zeniv-ca.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=a.darwish@linutronix.de \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=borntraeger@de.ibm.com \
    --cc=corbet@lwn.net \
    --cc=darrick.wong@oracle.com \
    --cc=ebiederm@xmission.com \
    --cc=ebiggers@google.com \
    --cc=gor@linux.ibm.com \
    --cc=hca@linux.ibm.com \
    --cc=ira.weiny@intel.com \
    --cc=justin.he@arm.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=peterz@infradead.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=senozhatsky@chromium.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@ftp.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.