| Message ID | 20231016-fototermin-umriss-59f1ea6c1fe6@brauner (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: simplify bdev_del_partition() | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 >
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>
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);
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(-)