| Message ID | 20230927-vfs-super-freeze-v1-7-ecc36d9ab4d9@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement freeze and thaw as holder operations | expand |
On Wed, Sep 27, 2023 at 03:21:20PM +0200, Christian Brauner wrote: > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > Documentation/filesystems/porting.rst | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst > index 4d05b9862451..fef97a2e6729 100644 > --- a/Documentation/filesystems/porting.rst > +++ b/Documentation/filesystems/porting.rst > @@ -1045,3 +1045,28 @@ filesystem type is now moved to a later point when the devices are closed: > As this is a VFS level change it has no practical consequences for filesystems > other than that all of them must use one of the provided kill_litter_super(), > kill_anon_super(), or kill_block_super() helpers. > + > +--- > + > +**mandatory** > + > +Block device freezing and thawing have been moved to holder operations. As we > +can now go straight from block devcie to superblock the get_active_super() s/devcie/device/ > +and bd_fsfreeze_sb members in struct block_device are gone. > + > +The bd_fsfreeze_mutex is gone as well since we can rely on the bd_holder_lock > +to protect against concurrent freeze and thaw. > + > +Before this change, get_active_super() would only be able to find the > +superblock of the main block device, i.e., the one stored in sb->s_bdev. Block > +device freezing now works for any block device owned by a given superblock, not > +just the main block device. You might want to document this new fs_holder_ops scheme: "Filesystems opening a block device must pass the super_block object and fs_holder_ops as the @holder and @hops parameters." Though TBH I see a surprising amount of fs code that doesn't do this, so perhaps it's not so mandatory? --D > + > +When thawing we now grab an active reference so we can hold bd_holder_lock > +across thaw without the risk of deadlocks (because the superblock goes away > +which would require us to take bd_holder_lock). That allows us to get rid of > +bd_fsfreeze_mutex. Currently we just reacquire s_umount after thaw_super() and > +drop the active reference we took before. This someone could grab an active > +reference before we dropped the last one. This shouldn't be an issue. If it > +turns out to be one we can reshuffle the code to simply hold s_umount when > +thaw_super() returns and drop the reference. > > -- > 2.34.1 >
On Wed 27-09-23 08:19:11, Darrick J. Wong wrote: > On Wed, Sep 27, 2023 at 03:21:20PM +0200, Christian Brauner wrote: > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > --- > > Documentation/filesystems/porting.rst | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst > > index 4d05b9862451..fef97a2e6729 100644 > > --- a/Documentation/filesystems/porting.rst > > +++ b/Documentation/filesystems/porting.rst > > @@ -1045,3 +1045,28 @@ filesystem type is now moved to a later point when the devices are closed: > > As this is a VFS level change it has no practical consequences for filesystems > > other than that all of them must use one of the provided kill_litter_super(), > > kill_anon_super(), or kill_block_super() helpers. > > + > > +--- > > + > > +**mandatory** > > + > > +Block device freezing and thawing have been moved to holder operations. As we > > +can now go straight from block devcie to superblock the get_active_super() > > s/devcie/device/ > > > +and bd_fsfreeze_sb members in struct block_device are gone. > > + > > +The bd_fsfreeze_mutex is gone as well since we can rely on the bd_holder_lock > > +to protect against concurrent freeze and thaw. > > + > > +Before this change, get_active_super() would only be able to find the > > +superblock of the main block device, i.e., the one stored in sb->s_bdev. Block > > +device freezing now works for any block device owned by a given superblock, not > > +just the main block device. > > You might want to document this new fs_holder_ops scheme: > > "Filesystems opening a block device must pass the super_block object > and fs_holder_ops as the @holder and @hops parameters." > > Though TBH I see a surprising amount of fs code that doesn't do this, so > perhaps it's not so mandatory? This is actually a good point. For the main device, fs/super.c takes care of this (perhaps except for btrfs). So this patch set should not regress anything. But for other devices such as the journal device or similar, passing proper holder and holder_ops from the filesystem is necessary. Honza
On Mon, Oct 02, 2023 at 06:45:24PM +0200, Jan Kara wrote: > > "Filesystems opening a block device must pass the super_block object > > and fs_holder_ops as the @holder and @hops parameters." > > > > Though TBH I see a surprising amount of fs code that doesn't do this, so > > perhaps it's not so mandatory? > > This is actually a good point. For the main device, fs/super.c takes care > of this (perhaps except for btrfs). So this patch set should not regress > anything. But for other devices such as the journal device or similar, > passing proper holder and holder_ops from the filesystem is necessary. It is is necessary to gain functionality where we call into the fs based on the block device. In the old get_super based world these never worked either as get_super was based on sb->s_dev only.
diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst index 4d05b9862451..fef97a2e6729 100644 --- a/Documentation/filesystems/porting.rst +++ b/Documentation/filesystems/porting.rst @@ -1045,3 +1045,28 @@ filesystem type is now moved to a later point when the devices are closed: As this is a VFS level change it has no practical consequences for filesystems other than that all of them must use one of the provided kill_litter_super(), kill_anon_super(), or kill_block_super() helpers. + +--- + +**mandatory** + +Block device freezing and thawing have been moved to holder operations. As we +can now go straight from block devcie to superblock the get_active_super() +and bd_fsfreeze_sb members in struct block_device are gone. + +The bd_fsfreeze_mutex is gone as well since we can rely on the bd_holder_lock +to protect against concurrent freeze and thaw. + +Before this change, get_active_super() would only be able to find the +superblock of the main block device, i.e., the one stored in sb->s_bdev. Block +device freezing now works for any block device owned by a given superblock, not +just the main block device. + +When thawing we now grab an active reference so we can hold bd_holder_lock +across thaw without the risk of deadlocks (because the superblock goes away +which would require us to take bd_holder_lock). That allows us to get rid of +bd_fsfreeze_mutex. Currently we just reacquire s_umount after thaw_super() and +drop the active reference we took before. This someone could grab an active +reference before we dropped the last one. This shouldn't be an issue. If it +turns out to be one we can reshuffle the code to simply hold s_umount when +thaw_super() returns and drop the reference.
Signed-off-by: Christian Brauner <brauner@kernel.org> --- Documentation/filesystems/porting.rst | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)