| Message ID | 20230518042323.663189-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] block: factor out a bd_end_claim helper from blkdev_put | expand |
On Thu 18-05-23 06:23:11, Christoph Hellwig wrote: > The long if/else chain obsfucates the actual logic. Tidy it up to be > more structured. Also drop the whole argument, as it can be trivially > derived from bdev using bdev_whole, and having the bdev_whole in the > function makes it easier to follow. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good to me. Just one nit below but regardless of how you decided feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> > +static bool bd_may_claim(struct block_device *bdev, void *holder) > { > - if (bdev->bd_holder == holder) > - return true; /* already a holder */ > - else if (bdev->bd_holder != NULL) > - return false; /* held by someone else */ > - else if (whole == bdev) > - return true; /* is a whole device which isn't held */ > - > - else if (whole->bd_holder == bd_may_claim) > - return true; /* is a partition of a device that is being partitioned */ > - else if (whole->bd_holder != NULL) > - return false; /* is a partition of a held device */ > - else > - return true; /* is a partition of an un-held device */ > + struct block_device *whole = bdev_whole(bdev); > + > + if (bdev->bd_holder) { > + /* > + * The same holder can always re-claim. > + */ > + if (bdev->bd_holder == holder) > + return true; > + return false; With this simple condition I'd just do: /* The same holder can always re-claim. */ return bdev->bd_holder == holder; Honza
On Tue, May 30, 2023 at 01:41:48PM +0200, Jan Kara wrote: > > + if (bdev->bd_holder) { > > + /* > > + * The same holder can always re-claim. > > + */ > > + if (bdev->bd_holder == holder) > > + return true; > > + return false; > > With this simple condition I'd just do: > /* The same holder can always re-claim. */ > return bdev->bd_holder == holder; As of this patch this makes sense, and I did in fact did it that way first. But once we start checking the holder ops we need the eplcicit conditional, so I decided to start out with this more verbose option to avoid churn later.
On Thu 01-06-23 10:11:05, Christoph Hellwig wrote: > On Tue, May 30, 2023 at 01:41:48PM +0200, Jan Kara wrote: > > > + if (bdev->bd_holder) { > > > + /* > > > + * The same holder can always re-claim. > > > + */ > > > + if (bdev->bd_holder == holder) > > > + return true; > > > + return false; > > > > With this simple condition I'd just do: > > /* The same holder can always re-claim. */ > > return bdev->bd_holder == holder; > > As of this patch this makes sense, and I did in fact did it that > way first. But once we start checking the holder ops we need > the eplcicit conditional, so I decided to start out with this more > verbose option to avoid churn later. Ah, OK. Honza
diff --git a/block/bdev.c b/block/bdev.c index 317bfd9cba40fa..080b5c83bfbc72 100644 --- a/block/bdev.c +++ b/block/bdev.c @@ -463,7 +463,6 @@ long nr_blockdev_pages(void) /** * bd_may_claim - test whether a block device can be claimed * @bdev: block device of interest - * @whole: whole block device containing @bdev, may equal @bdev * @holder: holder trying to claim @bdev * * Test whether @bdev can be claimed by @holder. @@ -474,22 +473,27 @@ long nr_blockdev_pages(void) * RETURNS: * %true if @bdev can be claimed, %false otherwise. */ -static bool bd_may_claim(struct block_device *bdev, struct block_device *whole, - void *holder) +static bool bd_may_claim(struct block_device *bdev, void *holder) { - if (bdev->bd_holder == holder) - return true; /* already a holder */ - else if (bdev->bd_holder != NULL) - return false; /* held by someone else */ - else if (whole == bdev) - return true; /* is a whole device which isn't held */ - - else if (whole->bd_holder == bd_may_claim) - return true; /* is a partition of a device that is being partitioned */ - else if (whole->bd_holder != NULL) - return false; /* is a partition of a held device */ - else - return true; /* is a partition of an un-held device */ + struct block_device *whole = bdev_whole(bdev); + + if (bdev->bd_holder) { + /* + * The same holder can always re-claim. + */ + if (bdev->bd_holder == holder) + return true; + return false; + } + + /* + * If the whole devices holder is set to bd_may_claim, a partition on + * the device is claimed, but not the whole device. + */ + if (whole != bdev && + whole->bd_holder && whole->bd_holder != bd_may_claim) + return false; + return true; } /** @@ -513,7 +517,7 @@ int bd_prepare_to_claim(struct block_device *bdev, void *holder) retry: spin_lock(&bdev_lock); /* if someone else claimed, fail */ - if (!bd_may_claim(bdev, whole, holder)) { + if (!bd_may_claim(bdev, holder)) { spin_unlock(&bdev_lock); return -EBUSY; } @@ -559,7 +563,7 @@ static void bd_finish_claiming(struct block_device *bdev, void *holder) struct block_device *whole = bdev_whole(bdev); spin_lock(&bdev_lock); - BUG_ON(!bd_may_claim(bdev, whole, holder)); + BUG_ON(!bd_may_claim(bdev, holder)); /* * Note that for a whole device bd_holders will be incremented twice, * and bd_holder will be set to bd_may_claim before being set to holder
The long if/else chain obsfucates the actual logic. Tidy it up to be more structured. Also drop the whole argument, as it can be trivially derived from bdev using bdev_whole, and having the bdev_whole in the function makes it easier to follow. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/bdev.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-)