All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Ian Kent <raven@themaw.net>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	autofs mailing list <autofs@vger.kernel.org>,
	Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Omar Sandoval <osandov@osandov.com>
Subject: Re: [PATCH 1/7] vfs - merge path_is_mountpoint() and path_is_mountpoint_rcu()
Date: Sat, 3 Dec 2016 05:13:22 +0000	[thread overview]
Message-ID: <20161203051322.GA24765@ZenIV.linux.org.uk> (raw)
In-Reply-To: <148029910861.27779.4517883721395202453.stgit@pluto.themaw.net>

	FWIW, I've folded that pile into vfs.git#work.autofs.

Problems:
	* (fixed) __path_is_mountpoint() should _not_ treat NULL from
__lookup_mnt() as "nothing's mounted there" until it has checked
that mount_lock hadn't been touched - mount --move on something unrelated
can race with lockless hash lookup and lead to false negatives.
	* linux/mount.h might be the wrong place for path_is_mountpoint().
Or it shouldn't be inlined.  I don't like the includes you've added there.
	* path_has_submounts() is broken.  At the very least, it's
AB-BA between mount_lock and rename_lock.  I would suggest trying to
put read_seqlock_excl(&mount_lock) around the call of d_walk() in there,
and using __lookup_mnt() in the callback (without retries on the mount_lock,
of course - read_seqlock_excl done on the outside is enough).  I'm not sure
if it won't cause trouble with contention, though; that needs testing.  As
it is, that function is broken in #work.autofs, same as it is in -mm and
-next.
	* the last one (propagation-related) is too ugly to live - at the
very least, its pieces should live in fs/pnode.c; exposing propagate_next()
is simply wrong.  I hadn't picked that one at all, and I would suggest
coordinating anything in that area with ebiederman - he has some work
around fs/pnode.c and you risk stepping on his toes.
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

WARNING: multiple messages have this Message-ID (diff)
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Ian Kent <raven@themaw.net>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	autofs mailing list <autofs@vger.kernel.org>,
	Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Omar Sandoval <osandov@osandov.com>
Subject: Re: [PATCH 1/7] vfs - merge path_is_mountpoint() and path_is_mountpoint_rcu()
Date: Sat, 3 Dec 2016 05:13:22 +0000	[thread overview]
Message-ID: <20161203051322.GA24765@ZenIV.linux.org.uk> (raw)
In-Reply-To: <148029910861.27779.4517883721395202453.stgit@pluto.themaw.net>

	FWIW, I've folded that pile into vfs.git#work.autofs.

Problems:
	* (fixed) __path_is_mountpoint() should _not_ treat NULL from
__lookup_mnt() as "nothing's mounted there" until it has checked
that mount_lock hadn't been touched - mount --move on something unrelated
can race with lockless hash lookup and lead to false negatives.
	* linux/mount.h might be the wrong place for path_is_mountpoint().
Or it shouldn't be inlined.  I don't like the includes you've added there.
	* path_has_submounts() is broken.  At the very least, it's
AB-BA between mount_lock and rename_lock.  I would suggest trying to
put read_seqlock_excl(&mount_lock) around the call of d_walk() in there,
and using __lookup_mnt() in the callback (without retries on the mount_lock,
of course - read_seqlock_excl done on the outside is enough).  I'm not sure
if it won't cause trouble with contention, though; that needs testing.  As
it is, that function is broken in #work.autofs, same as it is in -mm and
-next.
	* the last one (propagation-related) is too ugly to live - at the
very least, its pieces should live in fs/pnode.c; exposing propagate_next()
is simply wrong.  I hadn't picked that one at all, and I would suggest
coordinating anything in that area with ebiederman - he has some work
around fs/pnode.c and you risk stepping on his toes.

  parent reply	other threads:[~2016-12-03  5:13 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-28  2:11 [PATCH 1/7] vfs - merge path_is_mountpoint() and path_is_mountpoint_rcu() Ian Kent
2016-11-28  2:11 ` Ian Kent
2016-11-28  2:11 ` [PATCH 2/7] autofs - make struct path const in autofs4_dir_open() Ian Kent
2016-11-28  2:11 ` [PATCH 3/7] autofs - change struct path to const in autofs4_expire_wait() and autofs4_wait() Ian Kent
2016-11-28  2:11   ` Ian Kent
2016-11-28  2:12 ` [PATCH 4/7] vfs - change struct path to const in d_manage() Ian Kent
2016-11-28  2:12   ` Ian Kent
2016-11-28  2:12 ` [PATCH 5/7] vfs - constify path parameter of path_has_submounts() Ian Kent
2016-11-28  2:12 ` [PATCH 6/7] autofs - dont hold spin lock over direct mount expire Ian Kent
2016-11-28  2:12   ` Ian Kent
2016-11-28  2:12 ` [PATCH 7/7] vfs - make may_umount_tree() mount propogation aware Ian Kent
2016-11-30 22:22 ` [PATCH 1/7] vfs - merge path_is_mountpoint() and path_is_mountpoint_rcu() Andrew Morton
2016-11-30 22:22   ` Andrew Morton
2016-12-01  0:13   ` Ian Kent
2016-12-01  0:13     ` Ian Kent
2016-12-03  5:13 ` Al Viro [this message]
2016-12-03  5:13   ` Al Viro
2016-12-03 23:29   ` Al Viro
2016-12-03 23:29     ` Al Viro
2016-12-04  2:18     ` Ian Kent
2016-12-05  2:50       ` Ian Kent
2016-12-05  2:50         ` Ian Kent
2016-12-04  2:07   ` Ian Kent
2016-12-06  9:37   ` Ian Kent
2016-12-07 21:30     ` Eric W. Biederman
2016-12-08  3:29       ` Ian Kent
2016-12-08  3:29         ` Ian Kent
2016-12-08  4:28         ` Eric W. Biederman
2016-12-08  4:28           ` Eric W. Biederman
2016-12-08  6:18           ` Ian Kent
2016-12-08  6:18             ` Ian Kent

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=20161203051322.GA24765@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=akpm@linux-foundation.org \
    --cc=autofs@vger.kernel.org \
    --cc=ebiederm@xmission.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=osandov@osandov.com \
    --cc=raven@themaw.net \
    /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.