Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b98ab80d-1bc0-a378-d438-09ef8b375836@samsung.com>
Date:   Mon, 15 Aug 2022 15:38:29 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
        Thunderbird/91.11.0
From:   Pankaj Raghav <p.raghav@samsung.com>
Subject: Re: [PATCH v10 13/13] dm: add power-of-2 target for zoned devices
 with non power-of-2 zone sizes
To:     Damien Le Moal <damien.lemoal@opensource.wdc.com>,
        <snitzer@kernel.org>, <axboe@kernel.dk>, <hch@lst.de>,
        <agk@redhat.com>
CC:     <linux-block@vger.kernel.org>, <Johannes.Thumshirn@wdc.com>,
        <bvanassche@acm.org>, <matias.bjorling@wdc.com>, <hare@suse.de>,
        <gost.dev@samsung.com>, <linux-nvme@lists.infradead.org>,
        <jaegeuk@kernel.org>, <pankydev8@gmail.com>,
        <linux-kernel@vger.kernel.org>, <dm-devel@redhat.com>,
        Damien Le Moal <damien.lemoal@wdc.com>,
        Joel Granados <j.granados@samsung.com>
Content-Language: en-US
In-Reply-To: <bb4ab1f2-7e16-15a7-58b1-d37f6f3822fd@opensource.wdc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
CMS-TYPE: 201P
References: <20220811143043.126029-1-p.raghav@samsung.com>
        <CGME20220811143058eucas1p247291685ffff7a75186947fd30b5c13f@eucas1p2.samsung.com>
        <20220811143043.126029-14-p.raghav@samsung.com>
        <bb4ab1f2-7e16-15a7-58b1-d37f6f3822fd@opensource.wdc.com>
Precedence: bulk

Hi Damien,

>> +static int dm_po2z_map_read_emulated_area(struct dm_po2z_target *dmh,
>> +					  struct bio *bio)
> 
> This really should be called dm_po2z_handle_read() since it handles both cases
> of read commands, with and without the accept partial. Note that this is
> retesting the need for a split after that was already tested in dm_po2z_map()
> with bio_across_emulated_zone_area(). So the code should be better organized to
> avoid that repetition.
> 
> You can simplify the code by having bio_across_emulated_zone_area() return 0 for
> a bio that does not cross the zone capacity and return the number of sectors up
> to the zone capacity from the bio start if there is a cross. That value can then
The offset will also be zero if the BIO starts at the zone capacity and
dm_po2z_map will assume that the bio did not cross the emulated zone boundary.

> be used to call dm_accept_partial_bio() directly in dm_po2z_map() for read
> commands. That will make this function much simpler and essentially turn it into
> dm_po2z_read_zeroes().
> 
>> +{

What about something like this? We have one function:
bio_in_emulated_zone_area() that checks if a BIO is partly or completely in the
emulated zone area and also returns the offset from bio to the emulated zone
boundary (zone capacity of the device).

/**
 * bio_in_emulated_zone_area - check if bio is in the emulated zone area
 * @dmh:	pozone target data
 * @bio:	bio
 * @offset:	bio offset to emulated zone boundary
 *
 * Check if a @bio is partly or completely in the emulated zone area. If the
 * @bio is partly in the emulated zone area, @offset
 * can be used to split the @bio across the emulated zone boundary. @offset
 * will be negative if the @bio completely lies in the emulated area.
 *  */
static bool bio_in_emulated_zone_area(struct dm_po2z_target *dmh,
					  struct bio *bio, int *offset)
{
	unsigned int zone_idx = po2_zone_no(dmh, bio->bi_iter.bi_sector);
	sector_t nr_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
	sector_t relative_sect_in_zone =
		bio->bi_iter.bi_sector - (zone_idx << dmh->zone_size_po2_shift);

	*offset = dmh->zone_size - relative_sect_in_zone;

	return (relative_sect_in_zone + nr_sectors) > dmh->zone_size;
}

static int dm_po2z_read_zeroes(struct bio *bio)
{
	zero_fill_bio(bio);
	bio_endio(bio);
	return DM_MAPIO_SUBMITTED;
}

static int dm_po2z_remap_sector(struct dm_po2z_target *dmh, struct bio *bio)
{
	bio->bi_iter.bi_sector =
		target_to_device_sect(dmh, bio->bi_iter.bi_sector);
	return DM_MAPIO_REMAPPED;
}

static int dm_po2z_map(struct dm_target *ti, struct bio *bio)
{
	struct dm_po2z_target *dmh = ti->private;

	bio_set_dev(bio, dmh->dev->bdev);
	if (bio_sectors(bio) || op_is_zone_mgmt(bio_op(bio))) {
		int split_io_pos;
		if (!bio_in_emulated_zone_area(dmh, bio, &split_io_pos)) {
			return dm_po2z_remap_sector(dmh, bio);
		}
		/*
		 * Read operation on the emulated zone area (between zone capacity
		 * and zone size) will fill the bio with zeroes. Any other operation
		 * in the emulated area should return an error.
		 */
		if (bio_op(bio) == REQ_OP_READ) {
			if (split_io_pos > 0) {
				dm_accept_partial_bio(bio, split_io_pos);
				return dm_po2z_remap_sector(dmh, bio);
			}
			return dm_po2z_read_zeroes(bio);
		}
		return DM_MAPIO_KILL;
	}
	return DM_MAPIO_REMAPPED;
}

Let me know what you think.