On Tue 24-10-23 15:01:07, Christian Brauner wrote:
> Multiple people have balked at the the fact that
> super_lock{_shared,_excluse}() return booleans and even if they return
> false hold s_umount. So let's change them to only hold s_umount when
> true is returned and change the code accordingly.
>
> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
Yeah, it's easier to grasp calling convention I guess.
> @@ -1429,7 +1441,7 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> __releases(&bdev->bd_holder_lock)
> {
> struct super_block *sb = bdev->bd_holder;
> - bool born;
> + bool locked;
>
> lockdep_assert_held(&bdev->bd_holder_lock);
> lockdep_assert_not_held(&sb->s_umount);
> @@ -1441,9 +1453,8 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> spin_unlock(&sb_lock);
> mutex_unlock(&bdev->bd_holder_lock);
>
> - born = super_lock_shared(sb);
> - if (!born || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> - super_unlock_shared(sb);
> + locked = super_lock_shared(sb);
> + if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> put_super(sb);
> return NULL;
Here if locked == true but say !(sb->s_flags & SB_ACTIVE), we fail to
unlock the superblock now AFAICT.
> @@ -1959,9 +1970,11 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
> {
> int ret;
>
> + if (!super_lock_excl(sb)) {
> + WARN_ON_ONCE("Dying superblock while freezing!");
> + return -EINVAL;
> + }
> atomic_inc(&sb->s_active);
> - if (!super_lock_excl(sb))
> - WARN(1, "Dying superblock while freezing!");
>
> retry:
> if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
> @@ -2009,8 +2022,10 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
> /* Release s_umount to preserve sb_start_write -> s_umount ordering */
> super_unlock_excl(sb);
> sb_wait_write(sb, SB_FREEZE_WRITE);
> - if (!super_lock_excl(sb))
> - WARN(1, "Dying superblock while freezing!");
> + if (!super_lock_excl(sb)) {
> + WARN_ON_ONCE("Dying superblock while freezing!");
> + return -EINVAL;
> + }
And here if you really mean it with some kind of clean bail out, we should
somehow get rid of the s_active reference we have. But exactly because of
that getting super_lock_excl() failure here would be really weird...
Otherwise the patch looks good.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
