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@xxxxxxx>
> > ---
> > 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.
