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.
