| Message ID | 20241225100949.930897-4-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: improve write failure simulation | expand |
On 12/25/24 7:09 PM, Shin'ichiro Kawasaki wrote: > The previous commit modified bad blocks handling to do the partial IOs. > When such partial IOs happen for zoned null_blk devices, it is expected > that the write pointers also move partially. To test and debug partial > write by userland tools for zoned block devices, move write pointers > partially. > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > --- > drivers/block/null_blk/main.c | 5 ++++- > drivers/block/null_blk/null_blk.h | 6 ++++++ > drivers/block/null_blk/zoned.c | 10 ++++++++++ > 3 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index d155eb040077..1675dec0b0e6 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -1330,6 +1330,7 @@ static inline blk_status_t null_handle_throttled(struct nullb_cmd *cmd) > } > > static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd, > + enum req_op op, > sector_t sector, > sector_t nr_sectors) > { > @@ -1347,6 +1348,8 @@ static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd, > transfer_bytes = (first_bad - sector) << SECTOR_SHIFT; > __null_handle_rq(cmd, transfer_bytes); > } > + if (dev->zoned && op == REQ_OP_WRITE) Forgot REQ_OP_ZONE_APPEND ? > + null_move_zone_wp(dev, sector, first_bad - sector); > return BLK_STS_IOERR; > } > > @@ -1413,7 +1416,7 @@ blk_status_t null_process_cmd(struct nullb_cmd *cmd, enum req_op op, > blk_status_t ret; > > if (dev->badblocks.shift != -1) { > - ret = null_handle_badblocks(cmd, sector, nr_sectors); > + ret = null_handle_badblocks(cmd, op, sector, nr_sectors); > if (ret != BLK_STS_OK) > return ret; > } > diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h > index 6f9fe6171087..c6ceede691ba 100644 > --- a/drivers/block/null_blk/null_blk.h > +++ b/drivers/block/null_blk/null_blk.h > @@ -144,6 +144,8 @@ size_t null_zone_valid_read_len(struct nullb *nullb, > sector_t sector, unsigned int len); > ssize_t zone_cond_store(struct nullb_device *dev, const char *page, > size_t count, enum blk_zone_cond cond); > +void null_move_zone_wp(struct nullb_device *dev, sector_t zone_sector, > + sector_t nr_sectors); > #else > static inline int null_init_zoned_dev(struct nullb_device *dev, > struct queue_limits *lim) > @@ -173,6 +175,10 @@ static inline ssize_t zone_cond_store(struct nullb_device *dev, > { > return -EOPNOTSUPP; > } > +static inline void null_move_zone_wp(struct nullb_device *dev, > + sector_t zone_sector, sector_t nr_sectors) > +{ > +} > #define null_report_zones NULL > #endif /* CONFIG_BLK_DEV_ZONED */ > #endif /* __NULL_BLK_H */ > diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c > index 0d5f9bf95229..e2b8396aa318 100644 > --- a/drivers/block/null_blk/zoned.c > +++ b/drivers/block/null_blk/zoned.c > @@ -347,6 +347,16 @@ static blk_status_t null_check_zone_resources(struct nullb_device *dev, > } > } > > +void null_move_zone_wp(struct nullb_device *dev, sector_t zone_sector, > + sector_t nr_sectors) > +{ > + unsigned int zno = null_zone_no(dev, zone_sector); > + struct nullb_zone *zone = &dev->zones[zno]; > + > + if (zone->type != BLK_ZONE_TYPE_CONVENTIONAL) > + zone->wp += nr_sectors; > +} I do not think this is correct. We need to deal with the zone implicit open and zone resources as well, exactly like for a regular write. So it is not that simple. I think that overall, a simpler approach would be to reuse null_handle_badblocks() inside null_process_zoned_cmd(), similar to null_process_cmd(). If reusing null_handle_badblocks() inside null_process_zoned_cmd() is not possible, then let's write a null_handle_zone_badblocks() helper. > + > static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, > unsigned int nr_sectors, bool append) > {
On Jan 06, 2025 / 14:56, Damien Le Moal wrote: > On 12/25/24 7:09 PM, Shin'ichiro Kawasaki wrote: > > The previous commit modified bad blocks handling to do the partial IOs. > > When such partial IOs happen for zoned null_blk devices, it is expected > > that the write pointers also move partially. To test and debug partial > > write by userland tools for zoned block devices, move write pointers > > partially. > > > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > --- > > drivers/block/null_blk/main.c | 5 ++++- > > drivers/block/null_blk/null_blk.h | 6 ++++++ > > drivers/block/null_blk/zoned.c | 10 ++++++++++ > > 3 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > > index d155eb040077..1675dec0b0e6 100644 > > --- a/drivers/block/null_blk/main.c > > +++ b/drivers/block/null_blk/main.c > > @@ -1330,6 +1330,7 @@ static inline blk_status_t null_handle_throttled(struct nullb_cmd *cmd) > > } > > > > static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd, > > + enum req_op op, > > sector_t sector, > > sector_t nr_sectors) > > { > > @@ -1347,6 +1348,8 @@ static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd, > > transfer_bytes = (first_bad - sector) << SECTOR_SHIFT; > > __null_handle_rq(cmd, transfer_bytes); > > } > > + if (dev->zoned && op == REQ_OP_WRITE) > > Forgot REQ_OP_ZONE_APPEND ? Actually, null_process_zoned_cmd() handles both REQ_OP_WRITE and REQ_OP_ZONE_APPEND in the same way, and both case results in op == REQ_OP_WRITE here. Anyway, this if branch will not be required in the v3 series since this patch is totally rewritten to follow your the other suggestion. > > > + null_move_zone_wp(dev, sector, first_bad - sector); > > return BLK_STS_IOERR; > > } > > > > @@ -1413,7 +1416,7 @@ blk_status_t null_process_cmd(struct nullb_cmd *cmd, enum req_op op, > > blk_status_t ret; > > > > if (dev->badblocks.shift != -1) { > > - ret = null_handle_badblocks(cmd, sector, nr_sectors); > > + ret = null_handle_badblocks(cmd, op, sector, nr_sectors); > > if (ret != BLK_STS_OK) > > return ret; > > } > > diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h > > index 6f9fe6171087..c6ceede691ba 100644 > > --- a/drivers/block/null_blk/null_blk.h > > +++ b/drivers/block/null_blk/null_blk.h > > @@ -144,6 +144,8 @@ size_t null_zone_valid_read_len(struct nullb *nullb, > > sector_t sector, unsigned int len); > > ssize_t zone_cond_store(struct nullb_device *dev, const char *page, > > size_t count, enum blk_zone_cond cond); > > +void null_move_zone_wp(struct nullb_device *dev, sector_t zone_sector, > > + sector_t nr_sectors); > > #else > > static inline int null_init_zoned_dev(struct nullb_device *dev, > > struct queue_limits *lim) > > @@ -173,6 +175,10 @@ static inline ssize_t zone_cond_store(struct nullb_device *dev, > > { > > return -EOPNOTSUPP; > > } > > +static inline void null_move_zone_wp(struct nullb_device *dev, > > + sector_t zone_sector, sector_t nr_sectors) > > +{ > > +} > > #define null_report_zones NULL > > #endif /* CONFIG_BLK_DEV_ZONED */ > > #endif /* __NULL_BLK_H */ > > diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c > > index 0d5f9bf95229..e2b8396aa318 100644 > > --- a/drivers/block/null_blk/zoned.c > > +++ b/drivers/block/null_blk/zoned.c > > @@ -347,6 +347,16 @@ static blk_status_t null_check_zone_resources(struct nullb_device *dev, > > } > > } > > > > +void null_move_zone_wp(struct nullb_device *dev, sector_t zone_sector, > > + sector_t nr_sectors) > > +{ > > + unsigned int zno = null_zone_no(dev, zone_sector); > > + struct nullb_zone *zone = &dev->zones[zno]; > > + > > + if (zone->type != BLK_ZONE_TYPE_CONVENTIONAL) > > + zone->wp += nr_sectors; > > +} > > I do not think this is correct. We need to deal with the zone implicit open and > zone resources as well, exactly like for a regular write. So it is not that simple. > > I think that overall, a simpler approach would be to reuse > null_handle_badblocks() inside null_process_zoned_cmd(), similar to > null_process_cmd(). If reusing null_handle_badblocks() inside > null_process_zoned_cmd() is not possible, then let's write a > null_handle_zone_badblocks() helper. Right, I overlooked the zone resource management. I think we can reuse null_handle_badblolcks() from null_zone_write(). Please review v3 that I will post soon.
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index d155eb040077..1675dec0b0e6 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -1330,6 +1330,7 @@ static inline blk_status_t null_handle_throttled(struct nullb_cmd *cmd) } static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd, + enum req_op op, sector_t sector, sector_t nr_sectors) { @@ -1347,6 +1348,8 @@ static inline blk_status_t null_handle_badblocks(struct nullb_cmd *cmd, transfer_bytes = (first_bad - sector) << SECTOR_SHIFT; __null_handle_rq(cmd, transfer_bytes); } + if (dev->zoned && op == REQ_OP_WRITE) + null_move_zone_wp(dev, sector, first_bad - sector); return BLK_STS_IOERR; } @@ -1413,7 +1416,7 @@ blk_status_t null_process_cmd(struct nullb_cmd *cmd, enum req_op op, blk_status_t ret; if (dev->badblocks.shift != -1) { - ret = null_handle_badblocks(cmd, sector, nr_sectors); + ret = null_handle_badblocks(cmd, op, sector, nr_sectors); if (ret != BLK_STS_OK) return ret; } diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h index 6f9fe6171087..c6ceede691ba 100644 --- a/drivers/block/null_blk/null_blk.h +++ b/drivers/block/null_blk/null_blk.h @@ -144,6 +144,8 @@ size_t null_zone_valid_read_len(struct nullb *nullb, sector_t sector, unsigned int len); ssize_t zone_cond_store(struct nullb_device *dev, const char *page, size_t count, enum blk_zone_cond cond); +void null_move_zone_wp(struct nullb_device *dev, sector_t zone_sector, + sector_t nr_sectors); #else static inline int null_init_zoned_dev(struct nullb_device *dev, struct queue_limits *lim) @@ -173,6 +175,10 @@ static inline ssize_t zone_cond_store(struct nullb_device *dev, { return -EOPNOTSUPP; } +static inline void null_move_zone_wp(struct nullb_device *dev, + sector_t zone_sector, sector_t nr_sectors) +{ +} #define null_report_zones NULL #endif /* CONFIG_BLK_DEV_ZONED */ #endif /* __NULL_BLK_H */ diff --git a/drivers/block/null_blk/zoned.c b/drivers/block/null_blk/zoned.c index 0d5f9bf95229..e2b8396aa318 100644 --- a/drivers/block/null_blk/zoned.c +++ b/drivers/block/null_blk/zoned.c @@ -347,6 +347,16 @@ static blk_status_t null_check_zone_resources(struct nullb_device *dev, } } +void null_move_zone_wp(struct nullb_device *dev, sector_t zone_sector, + sector_t nr_sectors) +{ + unsigned int zno = null_zone_no(dev, zone_sector); + struct nullb_zone *zone = &dev->zones[zno]; + + if (zone->type != BLK_ZONE_TYPE_CONVENTIONAL) + zone->wp += nr_sectors; +} + static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, unsigned int nr_sectors, bool append) {
The previous commit modified bad blocks handling to do the partial IOs. When such partial IOs happen for zoned null_blk devices, it is expected that the write pointers also move partially. To test and debug partial write by userland tools for zoned block devices, move write pointers partially. Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- drivers/block/null_blk/main.c | 5 ++++- drivers/block/null_blk/null_blk.h | 6 ++++++ drivers/block/null_blk/zoned.c | 10 ++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-)