On Wed 05-07-23 15:46:19, Christian Brauner wrote:
> On Wed, Jul 05, 2023 at 03:00:33PM +0200, Jan Kara wrote:
> > On Tue 04-07-23 15:59:41, Christian Brauner wrote:
> > > On Tue, Jul 04, 2023 at 02:56:54PM +0200, Jan Kara wrote:
> > > > When we don't allow opening of mounted block devices for writing, bind
> > > > mounting is broken because the bind mount tries to open the block device
> > >
> > > Sorry, I'm going to be annoying now...
> > >
> > > Afaict, the analysis is misleading but I'm happy to be corrected ofc.
> >
> > I'm not sure what your objection exactly is. Probably I was imprecise in my
> > changelog description. What gets broken by not allowing RW open of a
> > mounted block device is:
> >
> > mount -t ext4 /dev/sda1 /mnt1
> > mount -t ext4 /dev/sda1 /mnt2
> >
> > The second mount should create another mount of the superblock created by
> > the first mount but before that is done, get_tree_bdev() tries to open the
> > block device and fails when only patches 1 & 2 are applied. This patch
> > fixes that.
>
> My objection is that this has nothing to do with mounts but with
> superblocks. :) No mount need exist for this issue to appear. And I would
> prefer if we keep superblock and mount separate as this leads to unclear
> analysis and changelogs.
>
> >
> > > Finding an existing superblock is independent of mounts. get_tree_bdev()
> > > and mount_bdev() are really only interested in finding a matching
> > > superblock independent of whether or not a mount for it already exists.
> > > IOW, if you had two filesystem contexts for the same block device with
> > > different mount options:
> > >
> > > T1 T2
> > > fd_fs = fsopen("ext4"); fd_fs = fsopen("ext4");
> > > fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda"); fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/dev/sda");
> > >
> > > // create superblock
> > > fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
> > > // finds superblock of T1 if opts are compatible
> > > fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...)
> > >
> > > you should have the issue that you're describing.
> >
> > Correct, this will get broken when not allowing RW open for mounted block
> > devices as well because the second fsconfig(fd_fs, FSCONFIG_CMD_CREATE,
> > ...) will fail to open the block device in get_tree_bdev(). But again this
> > patch should fix that.
> >
> > > But for neither of them does a mount already exist as the first mount
> > > here would only be created when:
> > >
> > > T1 T2
> > > fsmount(fd_fs); fsmount(fd_fs);
> > >
> > > is called at which point the whole superblock issue is already settled.
> > > Afterwards, both mounts of both T1 and T2 refer to the same superblock -
> > > as long as the fs and the mount options support this ofc.
> >
> > I guess the confusion comes from me calling "mount" an operation as
> > performed by the mount(8) command but which is in fact multiple operations
> > with the new mount API. Anyway, is the motivation of this patch clearer
> > now?
>
> I'm clear about what you're doing here. I would just like to not have
> mounts brought into the changelog. Even before the new mount api what
> you were describing was technically a superblock only issue. If someone
> reads the changelog I want them to be able to clearly see that this is a
> fix for superblocks, not mounts.
>
> Especially, since the code you touch really only has to to with
> superblocks.
> Let me - non ironically - return the question: Is my own request clearer
> now?
Yes, I understand now :). Thanks for explanation. I'll rephrase the
changelog to speak about superblocks.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
