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;
IronPort-SDR: QPCAinqKYkgwWONS598Cp1X3hJ7ygMz3ivgsEF3p6z8utUre011BSWuVtoCViecXF5Oj9a3Jja
 ltHOd2+FaEPVisEVsy7AxCpgR7fT6Fivt0tP2aB1wdK9VoUEX/pdNR7pFOQh2F5Ds2aFmnbLtu
 Fxm1a4RnVMY4JRUkMrBbw/AnKQ9iJGTKVeQkzMY0kGWzPYzHjmSxmZYUaHoudJ3RHXGTJynngB
 TVYgCK0tVsp9rVHaWqdbGBvplL8OZSTXQpFRLRvxKAiVpzGJYPwICOLU0SXLjceHuYOPb3y0u7
 pax0Vx77+Yg7InVPRung0g0T
IronPort-SDR: q5JvKtLj7EYeK3bnmJ2y6nykoFApnjzNXx9Rt0lHhSeRlYUzd6FYtddrCkQAlTUZkZNZc/Sj64
 5h6LpPx8Ezrib8yWtpx78NNIApVMt9xfC1v0VgDqADIDAkyEud6cmWgHosik0KpICPn8ch001U
 tWRENYia1xc02ZDgopxDjayLwKVG8EALxoat2eGFAuXpoqvSr5JLHNG6Mie9KhHwaHmMeD65RB
 s+UnvEQZ5QZul3P8VkY9uQMPAWBzfScEDTmS97iU5KRi1WR/ZF2bofNMzQ/WAI3d3fiSyzSTBE
 Hno=
WDCIronportException: Internal
Message-ID: <d4735d9e-31b6-0872-82a7-acbbd0cb5af5@opensource.wdc.com>
Date:   Mon, 15 Aug 2022 11:56:01 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.12.0
Subject: Re: [PATCH v10 13/13] dm: add power-of-2 target for zoned devices
 with non power-of-2 zone sizes
Content-Language: en-US
To:     Pankaj Raghav <p.raghav@samsung.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>
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>
 <b98ab80d-1bc0-a378-d438-09ef8b375836@samsung.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <b98ab80d-1bc0-a378-d438-09ef8b375836@samsung.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/08/15 6:38, Pankaj Raghav wrote:
> 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 =

Nit: Could simply call this sector_offset.

> 		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;

No need for the parenthesis.

> }
> 
> static int dm_po2z_read_zeroes(struct bio *bio)

Make that an inline.

> {
> 	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)

Inline this one too.

> {
> 	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;

Blank line needed here.

> 		if (!bio_in_emulated_zone_area(dmh, bio, &split_io_pos)) {
> 			return dm_po2z_remap_sector(dmh, bio);
> 		}

No need for the curly brackets.

> 		/*
> 		 * 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.

I think this is way better. But I would still reorganize this like this:

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

        bio_set_dev(bio, dmh->dev->bdev);

        if (op_is_zone_mgmt(bio_op(bio)))
                return dm_po2z_remap_sector(dmh, bio);

        if (!bio_sectors(bio))
                return DM_MAPIO_REMAPPED;

        /*
         * 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_in_emulated_zone_area(dmh, bio, &split_io_pos))
                return dm_po2z_remap_sector(dmh, bio);

        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;
}

I find the code easier to follow this way.


-- 
Damien Le Moal
Western Digital Research