* [PATCH] null_blk: synchronization fix for zoned device [not found] <CGME20200924062620epcas5p2f0468a1f41d4177ce15727c519bfbad5@epcas5p2.samsung.com> @ 2020-09-24 6:21 ` Kanchan Joshi 2020-09-24 8:41 ` Damien Le Moal 0 siblings, 1 reply; 3+ messages in thread From: Kanchan Joshi @ 2020-09-24 6:21 UTC (permalink / raw) To: axboe, Damien.LeMoal Cc: linux-kernel, linux-block, selvakuma.s1, nj.shetty, javier.gonz, Kanchan Joshi Parallel write,read,zone-mgmt operations accessing/altering zone state and write-pointer may get into race. Avoid the situation by using a new spinlock for zoned device. Concurrent zone-appends (on a zone) returning same write-pointer issue is also avoided using this lock. Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> --- drivers/block/null_blk.h | 1 + drivers/block/null_blk_zoned.c | 84 +++++++++++++++++++++++----------- 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h index daed4a9c3436..b3f4d62e7c38 100644 --- a/drivers/block/null_blk.h +++ b/drivers/block/null_blk.h @@ -44,6 +44,7 @@ struct nullb_device { unsigned int nr_zones; struct blk_zone *zones; sector_t zone_size_sects; + spinlock_t zlock; unsigned long size; /* device size in MB */ unsigned long completion_nsec; /* time in ns to complete a request */ diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c index 3d25c9ad2383..04fbf267703a 100644 --- a/drivers/block/null_blk_zoned.c +++ b/drivers/block/null_blk_zoned.c @@ -45,6 +45,7 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q) if (!dev->zones) return -ENOMEM; + spin_lock_init(&dev->zlock); if (dev->zone_nr_conv >= dev->nr_zones) { dev->zone_nr_conv = dev->nr_zones - 1; pr_info("changed the number of conventional zones to %u", @@ -124,6 +125,7 @@ int null_report_zones(struct gendisk *disk, sector_t sector, nr_zones = min(nr_zones, dev->nr_zones - first_zone); trace_nullb_report_zones(nullb, nr_zones); + spin_lock_irq(&dev->zlock); for (i = 0; i < nr_zones; i++) { /* * Stacked DM target drivers will remap the zone information by @@ -134,10 +136,13 @@ int null_report_zones(struct gendisk *disk, sector_t sector, memcpy(&zone, &dev->zones[first_zone + i], sizeof(struct blk_zone)); error = cb(&zone, i, data); - if (error) + if (error) { + spin_unlock_irq(&dev->zlock); return error; + } } + spin_unlock_irq(&dev->zlock); return nr_zones; } @@ -147,16 +152,24 @@ size_t null_zone_valid_read_len(struct nullb *nullb, struct nullb_device *dev = nullb->dev; struct blk_zone *zone = &dev->zones[null_zone_no(dev, sector)]; unsigned int nr_sectors = len >> SECTOR_SHIFT; + size_t ret = 0; + spin_lock_irq(&dev->zlock); /* Read must be below the write pointer position */ if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || - sector + nr_sectors <= zone->wp) - return len; + sector + nr_sectors <= zone->wp) { + ret = len; + goto out_unlock; + } if (sector > zone->wp) - return 0; + goto out_unlock; + + ret = (zone->wp - sector) << SECTOR_SHIFT; - return (zone->wp - sector) << SECTOR_SHIFT; +out_unlock: + spin_unlock_irq(&dev->zlock); + return ret; } static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, @@ -165,17 +178,19 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, struct nullb_device *dev = cmd->nq->dev; unsigned int zno = null_zone_no(dev, sector); struct blk_zone *zone = &dev->zones[zno]; - blk_status_t ret; + blk_status_t ret = BLK_STS_OK; trace_nullb_zone_op(cmd, zno, zone->cond); if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) return null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors); + spin_lock_irq(&dev->zlock); switch (zone->cond) { case BLK_ZONE_COND_FULL: /* Cannot write to a full zone */ - return BLK_STS_IOERR; + ret = BLK_STS_IOERR; + break; case BLK_ZONE_COND_EMPTY: case BLK_ZONE_COND_IMP_OPEN: case BLK_ZONE_COND_EXP_OPEN: @@ -193,27 +208,33 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, else cmd->rq->__sector = sector; } else if (sector != zone->wp) { - return BLK_STS_IOERR; + ret = BLK_STS_IOERR; + break; } - if (zone->wp + nr_sectors > zone->start + zone->capacity) - return BLK_STS_IOERR; + if (zone->wp + nr_sectors > zone->start + zone->capacity) { + ret = BLK_STS_IOERR; + break; + } if (zone->cond != BLK_ZONE_COND_EXP_OPEN) zone->cond = BLK_ZONE_COND_IMP_OPEN; ret = null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors); if (ret != BLK_STS_OK) - return ret; + break; zone->wp += nr_sectors; if (zone->wp == zone->start + zone->capacity) zone->cond = BLK_ZONE_COND_FULL; - return BLK_STS_OK; + break; default: /* Invalid zone condition */ - return BLK_STS_IOERR; + ret = BLK_STS_IOERR; } + + spin_unlock_irq(&dev->zlock); + return ret; } static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, @@ -223,7 +244,9 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, unsigned int zone_no = null_zone_no(dev, sector); struct blk_zone *zone = &dev->zones[zone_no]; size_t i; + blk_status_t ret = BLK_STS_OK; + spin_lock_irq(&dev->zlock); switch (op) { case REQ_OP_ZONE_RESET_ALL: for (i = 0; i < dev->nr_zones; i++) { @@ -234,25 +257,29 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, } break; case REQ_OP_ZONE_RESET: - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) - return BLK_STS_IOERR; + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) { + ret = BLK_STS_IOERR; + break; + } zone->cond = BLK_ZONE_COND_EMPTY; zone->wp = zone->start; break; case REQ_OP_ZONE_OPEN: - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) - return BLK_STS_IOERR; - if (zone->cond == BLK_ZONE_COND_FULL) - return BLK_STS_IOERR; + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || + zone->cond == BLK_ZONE_COND_FULL) { + ret = BLK_STS_IOERR; + break; + } zone->cond = BLK_ZONE_COND_EXP_OPEN; break; case REQ_OP_ZONE_CLOSE: - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) - return BLK_STS_IOERR; - if (zone->cond == BLK_ZONE_COND_FULL) - return BLK_STS_IOERR; + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || + zone->cond == BLK_ZONE_COND_FULL) { + ret = BLK_STS_IOERR; + break; + } if (zone->wp == zone->start) zone->cond = BLK_ZONE_COND_EMPTY; @@ -260,18 +287,21 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, zone->cond = BLK_ZONE_COND_CLOSED; break; case REQ_OP_ZONE_FINISH: - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) - return BLK_STS_IOERR; + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) { + ret = BLK_STS_IOERR; + break; + } zone->cond = BLK_ZONE_COND_FULL; zone->wp = zone->start + zone->len; break; default: - return BLK_STS_NOTSUPP; + ret = BLK_STS_NOTSUPP; } + spin_unlock_irq(&dev->zlock); trace_nullb_zone_op(cmd, zone_no, zone->cond); - return BLK_STS_OK; + return ret; } blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_opf op, -- 2.25.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] null_blk: synchronization fix for zoned device 2020-09-24 6:21 ` [PATCH] null_blk: synchronization fix for zoned device Kanchan Joshi @ 2020-09-24 8:41 ` Damien Le Moal 2020-09-24 17:22 ` Kanchan Joshi 0 siblings, 1 reply; 3+ messages in thread From: Damien Le Moal @ 2020-09-24 8:41 UTC (permalink / raw) To: Kanchan Joshi, axboe Cc: linux-kernel, linux-block, selvakuma.s1, nj.shetty, javier.gonz On 2020/09/24 15:26, Kanchan Joshi wrote: > Parallel write,read,zone-mgmt operations accessing/altering zone state > and write-pointer may get into race. Avoid the situation by using a new > spinlock for zoned device. > Concurrent zone-appends (on a zone) returning same write-pointer issue > is also avoided using this lock. > > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > --- > drivers/block/null_blk.h | 1 + > drivers/block/null_blk_zoned.c | 84 +++++++++++++++++++++++----------- > 2 files changed, 58 insertions(+), 27 deletions(-) > > diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h > index daed4a9c3436..b3f4d62e7c38 100644 > --- a/drivers/block/null_blk.h > +++ b/drivers/block/null_blk.h > @@ -44,6 +44,7 @@ struct nullb_device { > unsigned int nr_zones; > struct blk_zone *zones; > sector_t zone_size_sects; > + spinlock_t zlock; Could you change that to "zone_lock" to be consistent with the other zone related field names which all spell out "zone" ? > > unsigned long size; /* device size in MB */ > unsigned long completion_nsec; /* time in ns to complete a request */ > diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c > index 3d25c9ad2383..04fbf267703a 100644 > --- a/drivers/block/null_blk_zoned.c > +++ b/drivers/block/null_blk_zoned.c > @@ -45,6 +45,7 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q) > if (!dev->zones) > return -ENOMEM; > > + spin_lock_init(&dev->zlock); > if (dev->zone_nr_conv >= dev->nr_zones) { > dev->zone_nr_conv = dev->nr_zones - 1; > pr_info("changed the number of conventional zones to %u", > @@ -124,6 +125,7 @@ int null_report_zones(struct gendisk *disk, sector_t sector, > nr_zones = min(nr_zones, dev->nr_zones - first_zone); > trace_nullb_report_zones(nullb, nr_zones); > > + spin_lock_irq(&dev->zlock); > for (i = 0; i < nr_zones; i++) { > /* > * Stacked DM target drivers will remap the zone information by > @@ -134,10 +136,13 @@ int null_report_zones(struct gendisk *disk, sector_t sector, > memcpy(&zone, &dev->zones[first_zone + i], > sizeof(struct blk_zone)); > error = cb(&zone, i, data); The cb() here may sleep etc. So you cannot have the zone lock around it. Taking the lock around the memcpy only is enough. > - if (error) > + if (error) { > + spin_unlock_irq(&dev->zlock); > return error; > + } > } > > + spin_unlock_irq(&dev->zlock); > return nr_zones; > } > > @@ -147,16 +152,24 @@ size_t null_zone_valid_read_len(struct nullb *nullb, > struct nullb_device *dev = nullb->dev; > struct blk_zone *zone = &dev->zones[null_zone_no(dev, sector)]; > unsigned int nr_sectors = len >> SECTOR_SHIFT; > + size_t ret = 0; Please call that valid_len or something more representative of the value. > > + spin_lock_irq(&dev->zlock); > /* Read must be below the write pointer position */ > if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || > - sector + nr_sectors <= zone->wp) > - return len; > + sector + nr_sectors <= zone->wp) { > + ret = len; > + goto out_unlock; > + } > > if (sector > zone->wp) > - return 0; > + goto out_unlock; > + > + ret = (zone->wp - sector) << SECTOR_SHIFT; > > - return (zone->wp - sector) << SECTOR_SHIFT; > +out_unlock: > + spin_unlock_irq(&dev->zlock); > + return ret; > } > > static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, > @@ -165,17 +178,19 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, > struct nullb_device *dev = cmd->nq->dev; > unsigned int zno = null_zone_no(dev, sector); > struct blk_zone *zone = &dev->zones[zno]; > - blk_status_t ret; > + blk_status_t ret = BLK_STS_OK; > > trace_nullb_zone_op(cmd, zno, zone->cond); > > if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > return null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors); > > + spin_lock_irq(&dev->zlock); > switch (zone->cond) { > case BLK_ZONE_COND_FULL: > /* Cannot write to a full zone */ > - return BLK_STS_IOERR; > + ret = BLK_STS_IOERR; > + break; > case BLK_ZONE_COND_EMPTY: > case BLK_ZONE_COND_IMP_OPEN: > case BLK_ZONE_COND_EXP_OPEN: > @@ -193,27 +208,33 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, > else > cmd->rq->__sector = sector; > } else if (sector != zone->wp) { > - return BLK_STS_IOERR; > + ret = BLK_STS_IOERR; > + break; > } > > - if (zone->wp + nr_sectors > zone->start + zone->capacity) > - return BLK_STS_IOERR; > + if (zone->wp + nr_sectors > zone->start + zone->capacity) { > + ret = BLK_STS_IOERR; > + break; > + } > > if (zone->cond != BLK_ZONE_COND_EXP_OPEN) > zone->cond = BLK_ZONE_COND_IMP_OPEN; > > ret = null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors); > if (ret != BLK_STS_OK) > - return ret; > + break; > > zone->wp += nr_sectors; > if (zone->wp == zone->start + zone->capacity) > zone->cond = BLK_ZONE_COND_FULL; > - return BLK_STS_OK; > + break; > default: > /* Invalid zone condition */ > - return BLK_STS_IOERR; > + ret = BLK_STS_IOERR; > } > + > + spin_unlock_irq(&dev->zlock); > + return ret; > } > > static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, > @@ -223,7 +244,9 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, > unsigned int zone_no = null_zone_no(dev, sector); > struct blk_zone *zone = &dev->zones[zone_no]; > size_t i; > + blk_status_t ret = BLK_STS_OK; > > + spin_lock_irq(&dev->zlock); > switch (op) { > case REQ_OP_ZONE_RESET_ALL: > for (i = 0; i < dev->nr_zones; i++) { > @@ -234,25 +257,29 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, > } > break; > case REQ_OP_ZONE_RESET: > - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > - return BLK_STS_IOERR; > + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) { > + ret = BLK_STS_IOERR; > + break; > + } > > zone->cond = BLK_ZONE_COND_EMPTY; > zone->wp = zone->start; > break; > case REQ_OP_ZONE_OPEN: > - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > - return BLK_STS_IOERR; > - if (zone->cond == BLK_ZONE_COND_FULL) > - return BLK_STS_IOERR; > + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || > + zone->cond == BLK_ZONE_COND_FULL) { > + ret = BLK_STS_IOERR; > + break; > + } > > zone->cond = BLK_ZONE_COND_EXP_OPEN; > break; > case REQ_OP_ZONE_CLOSE: > - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > - return BLK_STS_IOERR; > - if (zone->cond == BLK_ZONE_COND_FULL) > - return BLK_STS_IOERR; > + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || > + zone->cond == BLK_ZONE_COND_FULL) { > + ret = BLK_STS_IOERR; > + break; > + } > > if (zone->wp == zone->start) > zone->cond = BLK_ZONE_COND_EMPTY; > @@ -260,18 +287,21 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, > zone->cond = BLK_ZONE_COND_CLOSED; > break; > case REQ_OP_ZONE_FINISH: > - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > - return BLK_STS_IOERR; > + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) { > + ret = BLK_STS_IOERR; > + break; > + } > > zone->cond = BLK_ZONE_COND_FULL; > zone->wp = zone->start + zone->len; > break; > default: > - return BLK_STS_NOTSUPP; > + ret = BLK_STS_NOTSUPP; > } > > + spin_unlock_irq(&dev->zlock); > trace_nullb_zone_op(cmd, zone_no, zone->cond); > - return BLK_STS_OK; > + return ret; > } I think you can avoid all of these changes by taking the lock around the calls to null_zone_mgmt() and null_zone_write() in null_process_zoned_cmd(). That will make the patch a lot smaller and simplify maintenance. And even, I think that taking the lock on entry to null_process_zoned_cmd() and unlocking on return should even be simpler since that would cover reads too (valid read len). Only report zones would need special treatment. > > blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_opf op, > I think we also need this to have a Cc: stable and a "Fixes" tag too. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] null_blk: synchronization fix for zoned device 2020-09-24 8:41 ` Damien Le Moal @ 2020-09-24 17:22 ` Kanchan Joshi 0 siblings, 0 replies; 3+ messages in thread From: Kanchan Joshi @ 2020-09-24 17:22 UTC (permalink / raw) To: Damien Le Moal Cc: Kanchan Joshi, axboe, linux-kernel, linux-block, selvakuma.s1, nj.shetty, javier.gonz Thanks for the review. I'll apply this feedback in V2. On Thu, Sep 24, 2020 at 2:13 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On 2020/09/24 15:26, Kanchan Joshi wrote: > > Parallel write,read,zone-mgmt operations accessing/altering zone state > > and write-pointer may get into race. Avoid the situation by using a new > > spinlock for zoned device. > > Concurrent zone-appends (on a zone) returning same write-pointer issue > > is also avoided using this lock. > > > > Signed-off-by: Kanchan Joshi <joshi.k@samsung.com> > > --- > > drivers/block/null_blk.h | 1 + > > drivers/block/null_blk_zoned.c | 84 +++++++++++++++++++++++----------- > > 2 files changed, 58 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/block/null_blk.h b/drivers/block/null_blk.h > > index daed4a9c3436..b3f4d62e7c38 100644 > > --- a/drivers/block/null_blk.h > > +++ b/drivers/block/null_blk.h > > @@ -44,6 +44,7 @@ struct nullb_device { > > unsigned int nr_zones; > > struct blk_zone *zones; > > sector_t zone_size_sects; > > + spinlock_t zlock; > > Could you change that to "zone_lock" to be consistent with the other zone > related field names which all spell out "zone" ? > > > > > unsigned long size; /* device size in MB */ > > unsigned long completion_nsec; /* time in ns to complete a request */ > > diff --git a/drivers/block/null_blk_zoned.c b/drivers/block/null_blk_zoned.c > > index 3d25c9ad2383..04fbf267703a 100644 > > --- a/drivers/block/null_blk_zoned.c > > +++ b/drivers/block/null_blk_zoned.c > > @@ -45,6 +45,7 @@ int null_init_zoned_dev(struct nullb_device *dev, struct request_queue *q) > > if (!dev->zones) > > return -ENOMEM; > > > > + spin_lock_init(&dev->zlock); > > if (dev->zone_nr_conv >= dev->nr_zones) { > > dev->zone_nr_conv = dev->nr_zones - 1; > > pr_info("changed the number of conventional zones to %u", > > @@ -124,6 +125,7 @@ int null_report_zones(struct gendisk *disk, sector_t sector, > > nr_zones = min(nr_zones, dev->nr_zones - first_zone); > > trace_nullb_report_zones(nullb, nr_zones); > > > > + spin_lock_irq(&dev->zlock); > > for (i = 0; i < nr_zones; i++) { > > /* > > * Stacked DM target drivers will remap the zone information by > > @@ -134,10 +136,13 @@ int null_report_zones(struct gendisk *disk, sector_t sector, > > memcpy(&zone, &dev->zones[first_zone + i], > > sizeof(struct blk_zone)); > > error = cb(&zone, i, data); > > The cb() here may sleep etc. So you cannot have the zone lock around it. Taking > the lock around the memcpy only is enough. > > > - if (error) > > + if (error) { > > + spin_unlock_irq(&dev->zlock); > > return error; > > + } > > } > > > > + spin_unlock_irq(&dev->zlock); > > return nr_zones; > > } > > > > @@ -147,16 +152,24 @@ size_t null_zone_valid_read_len(struct nullb *nullb, > > struct nullb_device *dev = nullb->dev; > > struct blk_zone *zone = &dev->zones[null_zone_no(dev, sector)]; > > unsigned int nr_sectors = len >> SECTOR_SHIFT; > > + size_t ret = 0; > > Please call that valid_len or something more representative of the value. > > > > > + spin_lock_irq(&dev->zlock); > > /* Read must be below the write pointer position */ > > if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || > > - sector + nr_sectors <= zone->wp) > > - return len; > > + sector + nr_sectors <= zone->wp) { > > + ret = len; > > + goto out_unlock; > > + } > > > > if (sector > zone->wp) > > - return 0; > > + goto out_unlock; > > + > > + ret = (zone->wp - sector) << SECTOR_SHIFT; > > > > - return (zone->wp - sector) << SECTOR_SHIFT; > > +out_unlock: > > + spin_unlock_irq(&dev->zlock); > > + return ret; > > } > > > > static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, > > @@ -165,17 +178,19 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, > > struct nullb_device *dev = cmd->nq->dev; > > unsigned int zno = null_zone_no(dev, sector); > > struct blk_zone *zone = &dev->zones[zno]; > > - blk_status_t ret; > > + blk_status_t ret = BLK_STS_OK; > > > > trace_nullb_zone_op(cmd, zno, zone->cond); > > > > if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > > return null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors); > > > > + spin_lock_irq(&dev->zlock); > > switch (zone->cond) { > > case BLK_ZONE_COND_FULL: > > /* Cannot write to a full zone */ > > - return BLK_STS_IOERR; > > + ret = BLK_STS_IOERR; > > + break; > > case BLK_ZONE_COND_EMPTY: > > case BLK_ZONE_COND_IMP_OPEN: > > case BLK_ZONE_COND_EXP_OPEN: > > @@ -193,27 +208,33 @@ static blk_status_t null_zone_write(struct nullb_cmd *cmd, sector_t sector, > > else > > cmd->rq->__sector = sector; > > } else if (sector != zone->wp) { > > - return BLK_STS_IOERR; > > + ret = BLK_STS_IOERR; > > + break; > > } > > > > - if (zone->wp + nr_sectors > zone->start + zone->capacity) > > - return BLK_STS_IOERR; > > + if (zone->wp + nr_sectors > zone->start + zone->capacity) { > > + ret = BLK_STS_IOERR; > > + break; > > + } > > > > if (zone->cond != BLK_ZONE_COND_EXP_OPEN) > > zone->cond = BLK_ZONE_COND_IMP_OPEN; > > > > ret = null_process_cmd(cmd, REQ_OP_WRITE, sector, nr_sectors); > > if (ret != BLK_STS_OK) > > - return ret; > > + break; > > > > zone->wp += nr_sectors; > > if (zone->wp == zone->start + zone->capacity) > > zone->cond = BLK_ZONE_COND_FULL; > > - return BLK_STS_OK; > > + break; > > default: > > /* Invalid zone condition */ > > - return BLK_STS_IOERR; > > + ret = BLK_STS_IOERR; > > } > > + > > + spin_unlock_irq(&dev->zlock); > > + return ret; > > } > > > > static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, > > @@ -223,7 +244,9 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, > > unsigned int zone_no = null_zone_no(dev, sector); > > struct blk_zone *zone = &dev->zones[zone_no]; > > size_t i; > > + blk_status_t ret = BLK_STS_OK; > > > > + spin_lock_irq(&dev->zlock); > > switch (op) { > > case REQ_OP_ZONE_RESET_ALL: > > for (i = 0; i < dev->nr_zones; i++) { > > @@ -234,25 +257,29 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, > > } > > break; > > case REQ_OP_ZONE_RESET: > > - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > > - return BLK_STS_IOERR; > > + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) { > > + ret = BLK_STS_IOERR; > > + break; > > + } > > > > zone->cond = BLK_ZONE_COND_EMPTY; > > zone->wp = zone->start; > > break; > > case REQ_OP_ZONE_OPEN: > > - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > > - return BLK_STS_IOERR; > > - if (zone->cond == BLK_ZONE_COND_FULL) > > - return BLK_STS_IOERR; > > + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || > > + zone->cond == BLK_ZONE_COND_FULL) { > > + ret = BLK_STS_IOERR; > > + break; > > + } > > > > zone->cond = BLK_ZONE_COND_EXP_OPEN; > > break; > > case REQ_OP_ZONE_CLOSE: > > - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > > - return BLK_STS_IOERR; > > - if (zone->cond == BLK_ZONE_COND_FULL) > > - return BLK_STS_IOERR; > > + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL || > > + zone->cond == BLK_ZONE_COND_FULL) { > > + ret = BLK_STS_IOERR; > > + break; > > + } > > > > if (zone->wp == zone->start) > > zone->cond = BLK_ZONE_COND_EMPTY; > > @@ -260,18 +287,21 @@ static blk_status_t null_zone_mgmt(struct nullb_cmd *cmd, enum req_opf op, > > zone->cond = BLK_ZONE_COND_CLOSED; > > break; > > case REQ_OP_ZONE_FINISH: > > - if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) > > - return BLK_STS_IOERR; > > + if (zone->type == BLK_ZONE_TYPE_CONVENTIONAL) { > > + ret = BLK_STS_IOERR; > > + break; > > + } > > > > zone->cond = BLK_ZONE_COND_FULL; > > zone->wp = zone->start + zone->len; > > break; > > default: > > - return BLK_STS_NOTSUPP; > > + ret = BLK_STS_NOTSUPP; > > } > > > > + spin_unlock_irq(&dev->zlock); > > trace_nullb_zone_op(cmd, zone_no, zone->cond); > > - return BLK_STS_OK; > > + return ret; > > } > > I think you can avoid all of these changes by taking the lock around the calls > to null_zone_mgmt() and null_zone_write() in null_process_zoned_cmd(). That will > make the patch a lot smaller and simplify maintenance. And even, I think that > taking the lock on entry to null_process_zoned_cmd() and unlocking on return > should even be simpler since that would cover reads too (valid read len). Only > report zones would need special treatment. > > > > > blk_status_t null_process_zoned_cmd(struct nullb_cmd *cmd, enum req_opf op, > > > > I think we also need this to have a Cc: stable and a "Fixes" tag too. > > -- > Damien Le Moal > Western Digital Research -- Joshi ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-24 17:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20200924062620epcas5p2f0468a1f41d4177ce15727c519bfbad5@epcas5p2.samsung.com>
2020-09-24 6:21 ` [PATCH] null_blk: synchronization fix for zoned device Kanchan Joshi
2020-09-24 8:41 ` Damien Le Moal
2020-09-24 17:22 ` Kanchan Joshi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).