Re: [PATCH 3/3] blkid: support zone reset for wipefs

[Naohiro Aota <naohiro.aota@wdc.com>]

On Tue, Apr 13, 2021 at 10:47:00PM +0000, Damien Le Moal wrote:
> On 2021/04/14 7:11, Naohiro Aota wrote:
> > We cannot overwrite superblock magic in a sequential required zone. So,
> > wipefs cannot work as it is. Instead, this commit implements the wiping by
> > zone resetting.
> > 
> > Zone resetting must be done only for a sequential write zone. This is
> > checked by is_conventional().
> > 
> > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> > ---
> >  libblkid/src/probe.c | 65 ++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 59 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
> > index 102766e57aa0..7454a14bdfe6 100644
> > --- a/libblkid/src/probe.c
> > +++ b/libblkid/src/probe.c
> > @@ -107,6 +107,7 @@
> >  #include <stdint.h>
> >  #include <stdarg.h>
> >  #include <limits.h>
> > +#include <stdbool.h>
> >  
> >  #include "blkidP.h"
> >  #include "all-io.h"
> > @@ -1225,6 +1226,40 @@ int blkid_do_probe(blkid_probe pr)
> >  	return rc;
> >  }
> >  
> > +static int is_conventional(blkid_probe pr, uint64_t offset)
> > +{
> > +	struct blk_zone_report *rep = NULL;
> > +	size_t rep_size;
> > +	bool conventional;
> > +	int ret;
> > +
> > +	if (!pr->zone_size)
> > +		return 1;
> > +
> > +	rep_size = sizeof(struct blk_zone_report) + sizeof(struct blk_zone);
> > +	rep = malloc(rep_size);
> > +	if (!rep)
> > +		return -1;
> > +
> > +	memset(rep, 0, rep_size);
> 
> Use calloc() to get the zeroing done at the same time as allocation, may be ?

Will do.

> > +	rep->sector = (offset & pr->zone_size) >> 9;
> 
> Why the "& pr->zone_size" ? This seems wrong. To get the zone info of the zone
> containing offset, you can just have:
> 
> 	rep->sector = offset >> 9;
> 
> And if you want to align to the zone start, then:
> 
> 	rep->sector = (offset & (~(pr->zone_size - 1))) >> 9;

Ouch. I was silly and missed this bug because it was only doing reset
on zone #0 and #1. I'll introduce "zone_mask" and do the correct
calculation.

> > +	rep->nr_zones = 1;
> > +	ret = ioctl(blkid_probe_get_fd(pr), BLKREPORTZONE, rep);
> > +	if (ret) {
> > +		free(rep);
> > +		return -1;
> > +	}
> > +
> > +	if (rep->zones[0].type == BLK_ZONE_TYPE_CONVENTIONAL)
> > +		ret = 1;
> > +	else
> > +		ret = 0;
> > +
> > +	free(rep);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * blkid_do_wipe:
> >   * @pr: prober
> > @@ -1264,6 +1299,7 @@ int blkid_do_wipe(blkid_probe pr, int dryrun)
> >  	const char *off = NULL;
> >  	size_t len = 0;
> >  	uint64_t offset, magoff;
> > +	bool conventional;
> >  	char buf[BUFSIZ];
> >  	int fd, rc = 0;
> >  	struct blkid_chain *chn;
> > @@ -1299,6 +1335,11 @@ int blkid_do_wipe(blkid_probe pr, int dryrun)
> >  	if (len > sizeof(buf))
> >  		len = sizeof(buf);
> >  
> > +	rc = is_conventional(pr, offset);
> > +	if (rc < 0)
> > +		return rc;
> > +	conventional = rc == 1;
> > +
> >  	DBG(LOWPROBE, ul_debug(
> >  	    "do_wipe [offset=0x%"PRIx64" (%"PRIu64"), len=%zu, chain=%s, idx=%d, dryrun=%s]\n",
> >  	    offset, offset, len, chn->driver->name, chn->idx, dryrun ? "yes" : "not"));
> > @@ -1306,13 +1347,25 @@ int blkid_do_wipe(blkid_probe pr, int dryrun)
> >  	if (lseek(fd, offset, SEEK_SET) == (off_t) -1)
> >  		return -1;
> >  
> > -	memset(buf, 0, len);
> > -
> >  	if (!dryrun && len) {
> > -		/* wipen on device */
> > -		if (write_all(fd, buf, len))
> > -			return -1;
> > -		fsync(fd);
> > +		if (conventional) {
> > +			memset(buf, 0, len);
> > +
> > +			/* wipen on device */
> > +			if (write_all(fd, buf, len))
> > +				return -1;
> > +			fsync(fd);
> > +		} else {
> > +			struct blk_zone_range range = {
> > +			    (offset & pr->zone_size) >> 9,
> > +			    pr->zone_size >> 9,
> > +			};
> 
> Please add the field names for clarity and fix the sector position (it looks
> very wrong):
> 
> 			struct blk_zone_range range = {
> 			    .sector = (offset & (~(pr->zone_size - 1))) >> 9,
> 			    .nr_sectors = pr->zone_size >> 9,
> 			};

Will do the same.

> > +
> > +			rc = ioctl(fd, BLKRESETZONE, &range);
> > +			if (rc < 0)
> > +				return -1;
> > +		}
> > +
> >  		pr->flags &= ~BLKID_FL_MODIF_BUFF;	/* be paranoid */
> >  
> >  		return blkid_probe_step_back(pr);
> > 
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research