Re: [PATCH] bcachefs: Fix sysfs warning in fstests generic/730,731

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

[meta comment, Kent, I'm not getting your emails sent to me at all, they
aren't even showing up in the gmail spam box, so something is really off
on your server such that google is just rejecting them all?]

On Mon, Oct 14, 2024 at 02:51:23AM -0400, Kent Overstreet wrote:
> On Mon, Oct 14, 2024 at 08:34:06AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Oct 14, 2024 at 08:10:19AM +0200, Christoph Hellwig wrote:
> > > On Sat, Oct 12, 2024 at 02:42:39PM -0400, Kent Overstreet wrote:
> > > > sysfs warns if we're removing a symlink from a directory that's no
> > > > longer in sysfs; this is triggered by fstests generic/730, which
> > > > simulates hot removal of a block device.
> > > > 
> > > > This patch is however not a correct fix, since checking
> > > > kobj->state_in_sysfs on a kobj owned by another subsystem is racy.
> > > > 
> > > > A better fix would be to add the appropriate check to
> > > > sysfs_remove_link() - and sysfs_create_link() as well.
> > > 
> > > The proper fix is to not link to random other subsystems with
> > > object lifetimes you can't know.  I'm not sure why you think adding
> > > this link was ever allowed.
> > > 
> > 
> > Odd, I never got the original patch that was sent here in the first
> > place...
> > 
> > Anyway, Christoph is right, this patch isn't ok.  You can't link outside
> > of the subdirectory in which you control in sysfs without a whole lot of
> > special cases and control.  The use of sysfs for filesystems is almost
> > always broken and tricky and full of race conditions (see many past
> > threads about this.)  Ideally we would fix this up by offering common
> > code for filesystems to use for sysfs (like we do for the driver
> > subsystems), but no one has gotten around to it for various reasons.
> 
> There was already past precedent with the block/holder.c code, and
> userspace does depend on that for determining the topology of virtual
> block devices.

What tools use that?  What sysfs links are being created for it?

And yes, filesystems do poke around in sysfs, but they almost always do
so in a racy way, see this old link for examples of common problems:
	https://lore.kernel.org/all/20230406120716.80980-1-frank.li@vivo.com/#r

> And that really is what sysfs is for, determining the actual topology
> and relationships between various devices - so if there's a relationship
> between devices we need to be able to expose that.

I totally agree, that is what sysfs is for, but at the filesystem layer
you all are having to deal with "raw" kobjects and doing that gets
tricky and is easy to get wrong.

> Re: the safety issues, I don't agree - provided you have a stable
> reference to the underlying kobject, which we do, since we have the
> block device open. The race is only w.r.t. kobj->state_in_sysfs, and
> that could be handled easily within the sysfs/kobject code.

Handled how?

> > The only filesystem that I can see that attempts to do much like what
> > bcachefs does in sysfs is btrfs, but btrfs only seems to have one
> > symlink, while you have multiple ones pointing to the same block device.
> 
> Not sure where you're seeing that? It's just a single backreference from
> the block device to the filesystem object.

I see multiple symlinks being created in the code, I don't know what it
looks like on a running system, sorry.

> > I can't find any sysfs documentation in Documentation/ABI/ so I don't
> > really understand what it's attempting to do (and why isn't the tools
> > that check this screaming about that lack of documentation, that's
> > odd...)  Any hints as to what you are wishing to show here?
> 
> Basically, it's the cleanest way (by far) for userspace to look up the
> filesystem from the block device: given a path to a block device, stat
> it to get the major:minor, then try to open
> /sys/dev/block/major:minor/bcachefs/.

Can you document this properly in Documentation/ABI/ which is where all
sysfs files and symlinks are supposed to be documented?  We have a tool
that you can run at runtime to show all missing documentation entries,
scripts/get_abi.pl

> The alternative would be scanning through /proc/mounts, which is really
> nasty - the format isn't particularly cleanly specified, it's racy, and
> with containers systems are getting into the thousands of mounts these
> days.

How does all other filesystems do this?  Surely we are not relying on
each filesystem to create these symlinks, that's just not going to
work...

thanks,

greg k-h