Message-ID: <20240228-entworfen-verkabeln-a036afc976ec@brauner>
Date: Wed, 28 Feb 2024 13:01:00 +0100
From: Christian Brauner <brauner@...nel.org>
To: John Groves <John@...ves.net>
Cc: John Groves <jgroves@...ron.com>, Jonathan Corbet <corbet@....net>, 
	Dan Williams <dan.j.williams@...el.com>, Vishal Verma <vishal.l.verma@...el.com>, 
	Dave Jiang <dave.jiang@...el.com>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Jan Kara <jack@...e.cz>, Matthew Wilcox <willy@...radead.org>, linux-cxl@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	nvdimm@...ts.linux.dev, john@...alactic.com, Dave Chinner <david@...morbit.com>, 
	Christoph Hellwig <hch@...radead.org>, dave.hansen@...ux.intel.com, gregory.price@...verge.com
Subject: Re: [RFC PATCH 11/20] famfs: Add fs_context_operations

On Wed, Feb 28, 2024 at 11:07:20AM +0100, Christian Brauner wrote:
> > I wasn't aware of the new fsconfig interface. Is there documentation or a
> > file sytsem that already uses it that I should refer to? I didn't find an
> > obvious candidate, but it might be me. If it should be obvious from the
> > example above, tell me and I'll try harder.
> > 
> > My famfs code above was copied from ramfs. If you point me to 
> 
> Ok, but that's the wrong filesystem to use as a model imho. Because it
> really doesn't deal with devices at all. That's why it uses
> get_tree_nodev() with "nodev" as in "no device" kinda. So ramfs doesn't
> have any of these issues. Whereas your filesystems is dealing with
> devices dax (or pmem).
> 
> > documentation I might send you a ramfs fsconfig patch too :D.
> 
> So the manpages are at:
> 
> https://github.com/brauner/man-pages-md
> 
> But really, there shouldn't be anything that needs to change for ramfs.
> 
> > > What errno is EALREADY? Isn't that socket stuff. In any case, it seems
> > > you want EBUSY?
> > 
> > Thanks... That should probaby be EBUSY. But the whole famfs_context_list
> > should probably also be removed. More below...
> > 
> > > 
> > > But bigger picture I'm lost. And why do you keep that list based on
> > > strings? What if I do:
> > > 
> > > mount -t famfs /dev/pmem1234 /mnt # succeeds
> > > 
> > > mount -t famfs /dev/pmem1234 /opt # ah, fsck me, this fails.. But wait a minute....
> > > 
> > > mount --bind /dev/pmem1234 /evil-masterplan
> > > 
> > > mount -t famfs /evil-masterplan /opt # succeeds. YAY
> > > 
> > > I believe that would trivially defeat your check.
> > > 
> > 
> > And I suspect this is related to the get_tree issue you noticed below.
> > 
> > This famfs code was working in 6.5 without keeping the linked list of devices,
> > but in 6.6/6.7/6.8 it works provided you don't try to repeat a mount command
> > that has already succeeded. I'm not sure why 6.5 protected me from that,
> > but the later versions don't. In 6.6+ That hits a BUG_ON (have specifics on 
> > that but not handy right now).
> 
> get_tree_nodev() by default will always allocate a new superblock. This
> is how tmpfs and ramfs work. If you do:
> 
> mount -t tmpfs tmpfs /mnt
> mount -t tmpfs tmpfs /opt
> 
> You get two new, independent superblocks. This is what you want for
> these multi-instance filesystems: each new mount creates a new instance.
> 
> If famfs doesn't want to allow reusing devices - which I very much think
> it wants to prevent - then it cannot use get_tree_nodev() directly
> without having a hack like you did. Because you'll get a new superblock
> no problem. So the fact that it did work somehow likely was a bug in
> your code.
> 
> The reason your code causes crashes is very likely this:
> 
> struct famfs_fs_info *fsi = sb->s_fs_info;
> handlep = bdev_open_by_path(fc->source, FAMFS_BLKDEV_MODE, fsi, &fs_holder_ops);
> 
> If you look at Documentation/filesystems/porting.rst you should see that
> if you use @fs_holder_ops then your holder should be the struct
> super_block, not your personal fsinfo.
> 
> > So for a while we just removed repeated mount requests from the famfs smoke
> > tests, but eventually I implemented the list above, which - though you're right
> > it would be easy to circumvent and therefore is not right - it did solve the
> > problem that we were testing for.
> > 
> > I suspect that correctly handling get_tree might solve this problem.
> > 
> > Please assume that linked list will be removed - it was not the right solution.
> > 
> > More below...
> > 
> > > > +		}
> > > > +	}
> > > > +
> > > > +	list_add(&fsi->fsi_list, &famfs_context_list);
> > > > +	mutex_unlock(&famfs_context_mutex);
> > > > +
> > > > +	return get_tree_nodev(fc, famfs_fill_super);
> > > 
> > > So why isn't this using get_tree_bdev()? Note that a while ago I
> > > added FSCONFIG_CMD_CREAT_EXCL which prevents silent superblock reuse. To
> > > implement that I added fs_context->exclusive. If you unconditionally set
> > > fc->exclusive = 1 in your famfs_init_fs_context() and use
> > > get_tree_bdev() it will give you EBUSY if fc->source is already in use -
> > > including other famfs instances.
> > > 
> > > I also fail to yet understand how that function which actually opens the block
> > > device and gets the dax device figures into this. It's a bit hard to follow
> > > what's going on since you add all those unused functions and types so there's
> > > never a wider context to see that stuff in.
> > 
> > Clearly that's a bug in my code. That get_tree_nodev() is from ramfs, which
> > was the starting point for famfs.
> > 
> > I'm wondering if doing this correctly (get_tree_bdev() when it's pmem) would
> > have solved my double mount problem on 6.6 onward.
> > 
> > However, there's another wrinkle: I'm concluding
> > (see https://lore.kernel.org/linux-fsdevel/ups6cvjw6bx5m3hotn452brbbcgemnarsasre6ep2lbe4tpjsy@ezp6oh5c72ur/)
> > that famfs should drop block support and just work with /dev/dax. So famfs 
> > may be the first file system to be hosted on a character device? Certainly 
> > first on character dax. 
> 
> Ugh, ok. I defer to others whether that makes sense or not. It would be
> a lot easier for you if you used pmem block devices, I guess because it
> would be easy to detect reuse in common infrastructure.
> 
> But also, I'm looking at your code a bit closer. There's a bit of a
> wrinkle the way it's currently written...
> 
> Say someone went a bit weird and did:
> 
> mount -t xfs xfs /dev/sda /my/xfs-filesystem
> mknod DAX_DEVICE /my/xfs-filesystem/dax1234
> 
> and then did:
> 
> mount -t famfs famfs /my/xfs-filesystem/dax1234 /mnt
> 
> Internally in famfs you do:
> 
> fsi->dax_filp = filp_open(fc->source, O_RDWR, 0);
> 
> and you stash that file... Which means that you are pinning that xfs
> filesystems implicitly. IOW, if someone does:
> 
> umount /my/xfs-filesystem
> 
> they get EBUSY for completely opaque reasons. And if they did:
> 
> umount -l /my/xfs-filesystem
> 
> followed by mounting that xfs filesystem again they'd get the same
> superblock for that xfs filesystem.
> 
> What I'm trying to say is that I think you cannot pin another filesystem
> like this when you open that device.
> 
> IOW, you either need to stash the plain dax device or dax needs to
> become it's own tiny internal pseudo fs such that we can open dax
> devices internally just like files. Which might actually also be worth
> doing. But I'm not the maintainer of that.

Ah, I see it's already like that and I was looking at the wrong file.
Great! So in that case you could add helper to open dax devices as
files:

struct file *dax_file_open(struct dax_device *dev, int flags, /* other stuff */)
{
	/* open that thing */
        dax_file = alloc_file_pseudo(dax_inode, dax_vfsmnt, "", flags | O_LARGEFILE, &something_fops);
}

and then you can treat them as regular files without running into the
issues I pointed out.

Powered by blists - more mailing lists