Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes: > On Tue, Jan 03, 2017 at 04:17:14PM +1300, Eric W. Biederman wrote: >> Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes: >> >> > On Tue, Jan 03, 2017 at 01:51:36PM +1300, Eric W. Biederman wrote: >> > >> >> The only significant thing I see is that you have not taken the >> >> mount_lock on the path where new_mountpoint adds the new struct >> >> mountpoint into the mountpoint hash table. >> > >> > Umm... Point, but I really don't like that bouncing mount_lock up >> > and down there. It's not going to cause any serious overhead, >> > but it just looks ugly... ;-/ >> > >> > Let me think for a while... >> >> The other possibility is to grab namespace_sem in mntput_no_expire >> around the call of umount_mnt. That is the only path where >> put_mountpoint can be called where we are not holding namespace_sem. >> That works in the small but I haven't traced the callers of mntput and >> mntput_no_expire yet to see if it works in practice. > > No, that's a really bad idea. Final mntput should _not_ happen under > namespace_lock, but I don't want grabbing it in that place. Agreed. That just makes the code harder to maintain later on. > How about this instead: I really don't like the logic inlined as my patch to kill shadow mounts needs to call acquire a mountpoint which may not already have been allocated as well. Beyond that we can make the logic simpler by causing d_set_mounted to fail if the flag is already set and syncrhonize on that. Which means we don't have to verify the ordering between mount_lock and rename_lock (from d_set_mounted) is not a problem, which makes backports easier to verify. Patch follows. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
