Mike,
Thank you. I just posted a follow up patch with a fix for a
reference-after-free (see below). I tested without problems.
On 6/12/17 14:28, Mike Snitzer wrote:
> On Fri, Jun 09 2017 at 8:03P -0400,
> Mike Snitzer <snitzer@xxxxxxxxxx> wrote:
>
>> I've pushed to linux-dm.git's 'for-next' to get more test coverage;
>> doesn't mean that I consider dm-zoned's review complete.
>
> I rebased ontop of Jens' latest 'for-4.13/block' to pull in all of
> Christoph's error code rework.
>
> I had to fix various commits in your dm-zoned series. Please
> review/retest dm-zoned to verify I didn't break anything.
>
> Thanks,
> Mike
>
> FYI, here is the incremental diff for the dm-zoned commit:
>
> From: Mike Snitzer <snitzer@xxxxxxxxxx>
> Date: Mon, 12 Jun 2017 16:44:50 -0400
> Subject: [PATCH] dm zoned: switch over to 4.13 error codes
>
> ---
> drivers/md/dm-zoned-metadata.c | 6 +++---
> drivers/md/dm-zoned-target.c | 37 ++++++++++++++++++-------------------
> drivers/md/dm-zoned.h | 2 +-
> 3 files changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/md/dm-zoned-metadata.c b/drivers/md/dm-zoned-metadata.c
> index 4618441c..b8f7d1d 100644
> --- a/drivers/md/dm-zoned-metadata.c
> +++ b/drivers/md/dm-zoned-metadata.c
> @@ -691,14 +691,14 @@ static int dmz_log_dirty_mblocks(struct dmz_metadata *zmd,
> /*
> * Flush dirty metadata blocks.
> */
> -int dmz_flush_metadata(struct dmz_metadata *zmd)
> +blk_status_t dmz_flush_metadata(struct dmz_metadata *zmd)
> {
> struct dmz_mblock *mblk;
> struct list_head write_list;
> int ret;
>
> if (WARN_ON(!zmd))
> - return 0;
> + return BLK_STS_OK;
>
> INIT_LIST_HEAD(&write_list);
>
> @@ -769,7 +769,7 @@ int dmz_flush_metadata(struct dmz_metadata *zmd)
> dmz_unlock_flush(zmd);
> up_write(&zmd->mblk_sem);
>
> - return ret;
> + return errno_to_blk_status(ret);
> }
I changed this back to returning an errno and do the
errno_to_blk_status() conversion when calling dm_zoned_endio(). Doing
so, all other functions in dm-zoned-target.c returning an errno can be
left as is.
> /*
> diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
> index 54055f4..c9fdd02 100644
> --- a/drivers/md/dm-zoned-target.c
> +++ b/drivers/md/dm-zoned-target.c
> @@ -20,7 +20,7 @@ struct dmz_bioctx {
> struct dm_zone *zone;
> struct bio *bio;
> atomic_t ref;
> - int error;
> + blk_status_t error;
> };
>
> /*
> @@ -74,12 +74,12 @@ struct dmz_target {
> /*
> * Target BIO completion.
> */
> -static inline void dmz_bio_endio(struct bio *bio, int err)
> +static inline void dmz_bio_endio(struct bio *bio, blk_status_t *error)
> {
> struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
>
> - if (bioctx->error == 0 && err != 0)
> - bioctx->error = err;
> + if (bioctx->error == BLK_STS_OK && *error)
> + bioctx->error = *error;
> bio_endio(bio);
> }
I changed this to "blk_status_t error". No need for a pointer.
>
> @@ -90,10 +90,9 @@ static inline void dmz_bio_endio(struct bio *bio, int err)
> static void dmz_read_bio_end_io(struct bio *bio)
> {
> struct dmz_bioctx *bioctx = bio->bi_private;
> - int err = bio->bi_error;
>
> bio_put(bio);
> - dmz_bio_endio(bioctx->bio, err);
> + dmz_bio_endio(bioctx->bio, &bio->bi_status);
> }
There is a reference after free here. So we need to keep the on stack
status.
>
> /*
> @@ -393,7 +392,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
> struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
> struct dmz_metadata *zmd = dmz->metadata;
> struct dm_zone *zone;
> - int ret;
> + blk_status_t ret;
>
> /*
> * Write may trigger a zone allocation. So make sure the
> @@ -436,7 +435,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
> default:
> dmz_dev_err(dmz->dev, "Unsupported BIO operation 0x%x",
> bio_op(bio));
> - ret = -EIO;
> + ret = BLK_STS_IOERR;
> }
The conversion to blk_status_t is done when calling dm_zoned_endio()
after this. So I reverted this change.
>
> /*
> @@ -446,7 +445,7 @@ static void dmz_handle_bio(struct dmz_target *dmz, struct dm_chunk_work *cw,
> if (zone)
> dmz_put_chunk_mapping(zmd, zone);
> out:
> - dmz_bio_endio(bio, ret);
> + dmz_bio_endio(bio, &ret);
>
> dmz_unlock_metadata(zmd);
> }
> @@ -504,7 +503,7 @@ static void dmz_flush_work(struct work_struct *work)
> {
> struct dmz_target *dmz = container_of(work, struct dmz_target, flush_work.work);
> struct bio *bio;
> - int ret;
> + blk_status_t ret;
>
> /* Flush dirty metadata blocks */
> ret = dmz_flush_metadata(dmz->metadata);
> @@ -518,7 +517,7 @@ static void dmz_flush_work(struct work_struct *work)
> if (!bio)
> break;
>
> - dmz_bio_endio(bio, ret);
> + dmz_bio_endio(bio, &ret);
> }
>
> queue_delayed_work(dmz->flush_wq, &dmz->flush_work, DMZ_FLUSH_PERIOD);
> @@ -593,14 +592,14 @@ static int dmz_map(struct dm_target *ti, struct bio *bio)
>
> /* The BIO should be block aligned */
> if ((nr_sectors & DMZ_BLOCK_SECTORS_MASK) || (sector & DMZ_BLOCK_SECTORS_MASK))
> - return -EIO;
> + return DM_MAPIO_KILL;
>
> /* Initialize the BIO context */
> bioctx->target = dmz;
> bioctx->zone = NULL;
> bioctx->bio = bio;
> atomic_set(&bioctx->ref, 1);
> - bioctx->error = 0;
> + bioctx->error = BLK_STS_OK;
>
> /* Set the BIO pending in the flush list */
> if (bio_op(bio) == REQ_OP_FLUSH || (!nr_sectors && bio_op(bio) == REQ_OP_WRITE)) {
> @@ -626,30 +625,30 @@ static int dmz_map(struct dm_target *ti, struct bio *bio)
> /*
> * Completed target BIO processing.
> */
> -static int dmz_end_io(struct dm_target *ti, struct bio *bio, int err)
> +static int dmz_end_io(struct dm_target *ti, struct bio *bio, blk_status_t *error)
> {
> struct dmz_bioctx *bioctx = dm_per_bio_data(bio, sizeof(struct dmz_bioctx));
>
> - if (bioctx->error == 0 && err != 0)
> - bioctx->error = err;
> + if (bioctx->error == BLK_STS_OK && *error)
> + bioctx->error = *error;
>
> if (!atomic_dec_and_test(&bioctx->ref))
> return DM_ENDIO_INCOMPLETE;
>
> /* Done */
> - bio->bi_error = bioctx->error;
> + bio->bi_status = bioctx->error;
>
> if (bioctx->zone) {
> struct dm_zone *zone = bioctx->zone;
>
> - if (err && bio_op(bio) == REQ_OP_WRITE) {
> + if (*error && bio_op(bio) == REQ_OP_WRITE) {
> if (dmz_is_seq(zone))
> set_bit(DMZ_SEQ_WRITE_ERR, &zone->flags);
> }
> dmz_deactivate_zone(zone);
> }
>
> - return bioctx->error;
> + return DM_ENDIO_DONE;
> }
>
> /*
> diff --git a/drivers/md/dm-zoned.h b/drivers/md/dm-zoned.h
> index 12419f0..76ebcf8 100644
> --- a/drivers/md/dm-zoned.h
> +++ b/drivers/md/dm-zoned.h
> @@ -169,7 +169,7 @@ void dmz_lock_metadata(struct dmz_metadata *zmd);
> void dmz_unlock_metadata(struct dmz_metadata *zmd);
> void dmz_lock_flush(struct dmz_metadata *zmd);
> void dmz_unlock_flush(struct dmz_metadata *zmd);
> -int dmz_flush_metadata(struct dmz_metadata *zmd);
> +blk_status_t dmz_flush_metadata(struct dmz_metadata *zmd);
>
> unsigned int dmz_id(struct dmz_metadata *zmd, struct dm_zone *zone);
> sector_t dmz_start_sect(struct dmz_metadata *zmd, struct dm_zone *zone);
>
Best regards.
--
Damien Le Moal,
Western Digital Research
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
