[PATCH] block: simplify bdev_del_partition()

[PATCH] block: simplify bdev_del_partition()

[Christian Brauner]

BLKPG_DEL_PARTITION refuses to delete partitions that still have
openers, i.e., that has an elevated @bdev->bd_openers count. If a device
is claimed by setting @bdev->bd_holder and @bdev->bd_holder_ops
@bdev->bd_openers and @bdev->bd_holders are incremented.
@bdev->bd_openers is effectively guaranteed to be >= @bdev->bd_holders.
So as long as @bdev->bd_openers isn't zero we know that this partition
is still in active use and that there might still be @bdev->bd_holder
and @bdev->bd_holder_ops set.

The only current example is @fs_holder_ops for filesystems. But that
means bdev_mark_dead() which calls into
bdev->bd_holder_ops->mark_dead::fs_bdev_mark_dead() is a nop. As long as
there's an elevated @bdev->bd_openers count we can't delete the
partition and if there isn't an elevated @bdev->bd_openers count then
there's no @bdev->bd_holder or @bdev->bd_holder_ops.

So simply open-code what we need to do. This gets rid of one more
instance where we acquire s_umount under @disk->open_mutex.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Hey Jens,

This came out of an ongoing locking design discussion and is related
to what we currently have in vfs.super. So if everyone still agrees my
reasoning is right and you don't have big objections I'd take it through
there.

As usual, thanks to Jan and Christoph for good discussions here.

Thanks!
Christian
---
 block/partitions/core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index e137a87f4db0..b0585536b407 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -485,7 +485,18 @@ int bdev_del_partition(struct gendisk *disk, int partno)
 	if (atomic_read(&part->bd_openers))
 		goto out_unlock;
 
-	delete_partition(part);
+	/*
+	 * We verified that @part->bd_openers is zero above and so
+	 * @part->bd_holder{_ops} can't be set. And since we hold
+	 * @disk->open_mutex the device can't be claimed by anyone.
+	 *
+	 * So no need to call @part->bd_holder_ops->mark_dead() here.
+	 * Just delete the partition and invalidate it.
+	 */
+
+	remove_inode_hash(part->bd_inode);
+	invalidate_bdev(part);
+	drop_partition(part);
 	ret = 0;
 out_unlock:
 	mutex_unlock(&disk->open_mutex);
-- 
2.34.1

Re: [PATCH] block: simplify bdev_del_partition()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] block: simplify bdev_del_partition()

[Jan Kara]

On Mon 16-10-23 17:27:18, Christian Brauner wrote:
> BLKPG_DEL_PARTITION refuses to delete partitions that still have
> openers, i.e., that has an elevated @bdev->bd_openers count. If a device
> is claimed by setting @bdev->bd_holder and @bdev->bd_holder_ops
> @bdev->bd_openers and @bdev->bd_holders are incremented.
> @bdev->bd_openers is effectively guaranteed to be >= @bdev->bd_holders.
> So as long as @bdev->bd_openers isn't zero we know that this partition
> is still in active use and that there might still be @bdev->bd_holder
> and @bdev->bd_holder_ops set.
> 
> The only current example is @fs_holder_ops for filesystems. But that
> means bdev_mark_dead() which calls into
> bdev->bd_holder_ops->mark_dead::fs_bdev_mark_dead() is a nop. As long as
> there's an elevated @bdev->bd_openers count we can't delete the
> partition and if there isn't an elevated @bdev->bd_openers count then
> there's no @bdev->bd_holder or @bdev->bd_holder_ops.
> 
> So simply open-code what we need to do. This gets rid of one more
> instance where we acquire s_umount under @disk->open_mutex.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

BTW, now when there's only one delete_partition() caller, we could just
opencode it in its callsite...

								Honza

> ---
> Hey Jens,
> 
> This came out of an ongoing locking design discussion and is related
> to what we currently have in vfs.super. So if everyone still agrees my
> reasoning is right and you don't have big objections I'd take it through
> there.
> 
> As usual, thanks to Jan and Christoph for good discussions here.
> 
> Thanks!
> Christian
> ---
>  block/partitions/core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/block/partitions/core.c b/block/partitions/core.c
> index e137a87f4db0..b0585536b407 100644
> --- a/block/partitions/core.c
> +++ b/block/partitions/core.c
> @@ -485,7 +485,18 @@ int bdev_del_partition(struct gendisk *disk, int partno)
>  	if (atomic_read(&part->bd_openers))
>  		goto out_unlock;
>  
> -	delete_partition(part);
> +	/*
> +	 * We verified that @part->bd_openers is zero above and so
> +	 * @part->bd_holder{_ops} can't be set. And since we hold
> +	 * @disk->open_mutex the device can't be claimed by anyone.
> +	 *
> +	 * So no need to call @part->bd_holder_ops->mark_dead() here.
> +	 * Just delete the partition and invalidate it.
> +	 */
> +
> +	remove_inode_hash(part->bd_inode);
> +	invalidate_bdev(part);
> +	drop_partition(part);
>  	ret = 0;
>  out_unlock:
>  	mutex_unlock(&disk->open_mutex);
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] block: simplify bdev_del_partition()

[Jens Axboe]

On 10/16/23 9:27 AM, Christian Brauner wrote:
> BLKPG_DEL_PARTITION refuses to delete partitions that still have
> openers, i.e., that has an elevated @bdev->bd_openers count. If a device
> is claimed by setting @bdev->bd_holder and @bdev->bd_holder_ops
> @bdev->bd_openers and @bdev->bd_holders are incremented.
> @bdev->bd_openers is effectively guaranteed to be >= @bdev->bd_holders.
> So as long as @bdev->bd_openers isn't zero we know that this partition
> is still in active use and that there might still be @bdev->bd_holder
> and @bdev->bd_holder_ops set.
> 
> The only current example is @fs_holder_ops for filesystems. But that
> means bdev_mark_dead() which calls into
> bdev->bd_holder_ops->mark_dead::fs_bdev_mark_dead() is a nop. As long as
> there's an elevated @bdev->bd_openers count we can't delete the
> partition and if there isn't an elevated @bdev->bd_openers count then
> there's no @bdev->bd_holder or @bdev->bd_holder_ops.
> 
> So simply open-code what we need to do. This gets rid of one more
> instance where we acquire s_umount under @disk->open_mutex.

Reviwed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe