| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Aug 2022 11:56:01 -0700
From: Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To: Pankaj Raghav <p.raghav@...sung.com>, snitzer@...nel.org,
axboe@...nel.dk, hch@....de, agk@...hat.com
Cc: linux-block@...r.kernel.org, Johannes.Thumshirn@....com,
bvanassche@....org, matias.bjorling@....com, hare@...e.de,
gost.dev@...sung.com, linux-nvme@...ts.infradead.org,
jaegeuk@...nel.org, pankydev8@...il.com,
linux-kernel@...r.kernel.org, dm-devel@...hat.com,
Damien Le Moal <damien.lemoal@....com>,
Joel Granados <j.granados@...sung.com>
Subject: Re: [PATCH v10 13/13] dm: add power-of-2 target for zoned devices
with non power-of-2 zone sizes
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
Powered by blists - more mailing lists