[dm-devel] [PATCH 07/11] dm-zoned: use device as argument for bio handler functions
Hannes Reinecke
hare at suse.de
Tue Apr 7 08:12:57 UTC 2020
On 4/7/20 4:50 AM, Damien Le Moal wrote:
> On 2020/04/07 3:24, Hannes Reinecke wrote:
>> Instead of having each function to reference the device for
>> themselves add it as an argument to the function.
>> And replace the 'target' pointer in the bio context with the
>> device pointer.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>> ---
>> drivers/md/dm-zoned-target.c | 88 +++++++++++++++++++++---------------
>> 1 file changed, 52 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
>> index 02ee0ca1f0b0..8ed6d9f2df25 100644
>> --- a/drivers/md/dm-zoned-target.c
>> +++ b/drivers/md/dm-zoned-target.c
>> @@ -17,7 +17,7 @@
>> * Zone BIO context.
>> */
>> struct dmz_bioctx {
>> - struct dmz_target *target;
>> + struct dmz_dev *dev;
>> struct dm_zone *zone;
>> struct bio *bio;
>> refcount_t ref;
>> @@ -71,6 +71,11 @@ struct dmz_target {
>> */
>> #define DMZ_FLUSH_PERIOD (10 * HZ)
>>
>> +struct dmz_dev *dmz_sect_to_dev(struct dmz_target *dmz, sector_t sect)
>> +{
>> + return &dmz->dev[0];
>> +}
>
> I do not get it. Regardless of the chunk sector specified, always returning the
> first device seems wrong. If the sector belongs to a chunk mapped to a zone on
> the second device, things will break, no ?
>
This is just a stub for now, so that the actual patch introducing
several devices can fold into here and the code churn for the final
patch is reduced.
>> +
>> /*
>> * Target BIO completion.
>> */
>> @@ -81,7 +86,7 @@ static inline void dmz_bio_endio(struct bio *bio, blk_status_t status)
>> if (status != BLK_STS_OK && bio->bi_status == BLK_STS_OK)
>> bio->bi_status = status;
>> if (bio->bi_status != BLK_STS_OK)
>> - bioctx->target->dev->flags |= DMZ_CHECK_BDEV;
>> + bioctx->dev->flags |= DMZ_CHECK_BDEV;
>>
>> if (refcount_dec_and_test(&bioctx->ref)) {
>> struct dm_zone *zone = bioctx->zone;
>> @@ -114,18 +119,20 @@ static void dmz_clone_endio(struct bio *clone)
>> * Issue a clone of a target BIO. The clone may only partially process the
>> * original target BIO.
>> */
>> -static int dmz_submit_bio(struct dmz_target *dmz, struct dm_zone *zone,
>> - struct bio *bio, sector_t chunk_block,
>> - unsigned int nr_blocks)
>> +static int dmz_submit_bio(struct dmz_target *dmz, struct dmz_dev *dev,
>> + struct dm_zone *zone, struct bio *bio,
>> + sector_t chunk_block, unsigned int nr_blocks)
>
> dev could be inferred from the zone with dmz_zone_to_dev(), no ?
>
Yes, it could, but then I'll find myself calling dmz_zone_to_dev() on
every function in the call chain.
So I thought to pass in 'dev' directly to avoid that.
>> {
>> - struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
>> + struct dmz_bioctx *bioctx =
>> + dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
>> struct bio *clone;
>>
>> clone = bio_clone_fast(bio, GFP_NOIO, &dmz->bio_set);
>> if (!clone)
>> return -ENOMEM;
>>
>> - bio_set_dev(clone, dmz->dev->bdev);
>> + bio_set_dev(clone, dev->bdev);
>> + bioctx->dev = dev;
>> clone->bi_iter.bi_sector =
>> dmz_start_sect(dmz->metadata, zone) + dmz_blk2sect(chunk_block);
>> clone->bi_iter.bi_size = dmz_blk2sect(nr_blocks) << SECTOR_SHIFT;
>> @@ -162,8 +169,8 @@ static void dmz_handle_read_zero(struct dmz_target *dmz, struct bio *bio,
>> /*
>> * Process a read BIO.
>> */
>> -static int dmz_handle_read(struct dmz_target *dmz, struct dm_zone *zone,
>> - struct bio *bio)
>> +static int dmz_handle_read(struct dmz_target *dmz, struct dmz_dev *dev,
>> + struct dm_zone *zone, struct bio *bio)
>
> Same comment as above.
>
... which is why I did it ...
>> {
>> struct dmz_metadata *zmd = dmz->metadata;
>> sector_t chunk_block = dmz_chunk_block(zmd, dmz_bio_block(bio));
>> @@ -178,7 +185,7 @@ static int dmz_handle_read(struct dmz_target *dmz, struct dm_zone *zone,
>> return 0;
>> }
>>
>> - dmz_dev_debug(dmz->dev, "READ chunk %llu -> %s zone %u, block %llu, %u blocks",
>> + dmz_dev_debug(dev, "READ chunk %llu -> %s zone %u, block %llu, %u blocks",
>> (unsigned long long)dmz_bio_chunk(zmd, bio),
>> (dmz_is_rnd(zone) ? "RND" : "SEQ"),
>> dmz_id(zmd, zone),
>
> DMDEBUG to print the label ? or we could also have dmz_dev_debug() print format
> changed to something like: "%s (%s): ...", label_name, dev->bdev_name
>
As you've seen, I've not been very consistent when to use the device
name or the label. But indeed, using both as you suggested is a good idea.
I'll be modifying it for the next round.
>> @@ -218,7 +225,8 @@ static int dmz_handle_read(struct dmz_target *dmz, struct dm_zone *zone,
>> if (nr_blocks) {
>> /* Valid blocks found: read them */
>> nr_blocks = min_t(unsigned int, nr_blocks, end_block - chunk_block);
>> - ret = dmz_submit_bio(dmz, rzone, bio, chunk_block, nr_blocks);
>> + ret = dmz_submit_bio(dmz, dev, rzone, bio,
>> + chunk_block, nr_blocks);
>> if (ret)
>> return ret;
>> chunk_block += nr_blocks;
>> @@ -238,6 +246,7 @@ static int dmz_handle_read(struct dmz_target *dmz, struct dm_zone *zone,
>> * in place.
>> */
>> static int dmz_handle_direct_write(struct dmz_target *dmz,
>> + struct dmz_dev *dev,
>
> Use dmz_zone_to_dev() ?
>
>> struct dm_zone *zone, struct bio *bio,
>> sector_t chunk_block,
>> unsigned int nr_blocks)
>> @@ -250,7 +259,7 @@ static int dmz_handle_direct_write(struct dmz_target *dmz,
>> return -EROFS;
>>
>> /* Submit write */
>> - ret = dmz_submit_bio(dmz, zone, bio, chunk_block, nr_blocks);
>> + ret = dmz_submit_bio(dmz, dev, zone, bio, chunk_block, nr_blocks);
>> if (ret)
>> return ret;
>>
>> @@ -271,6 +280,7 @@ static int dmz_handle_direct_write(struct dmz_target *dmz,
>> * Called with @zone write locked.
>> */
>> static int dmz_handle_buffered_write(struct dmz_target *dmz,
>> + struct dmz_dev *dev,
>> struct dm_zone *zone, struct bio *bio,
>> sector_t chunk_block,
>> unsigned int nr_blocks)
>> @@ -288,7 +298,7 @@ static int dmz_handle_buffered_write(struct dmz_target *dmz,
>> return -EROFS;
>>
>> /* Submit write */
>> - ret = dmz_submit_bio(dmz, bzone, bio, chunk_block, nr_blocks);
>> + ret = dmz_submit_bio(dmz, dev, bzone, bio, chunk_block, nr_blocks);
>
> Yes, I think it would be far better to use dmz_zone_to_dev() instead of passing
> directly dev. That will avoid bugs like passing a wrong dev for a zone or wrong
> zone for a dev.
>
As mentioned above, yes, we could.
In fact, that's what I did originally.
But then I thought it easier to pass the device precisely to avoid
having to call dmz_zone_to_dev() in every function.
However, I'll see how things pan out and will be modifying it for the
next round.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare at suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
More information about the dm-devel
mailing list