Re: [PATCH] libblkid: implement zone-aware probing for HMZONED btrfs

From: David Sterba <dsterba@suse.cz>
To: Naohiro Aota <naohiro.aota@wdc.com>
Cc: Vyacheslav Dubeyko <slava@dubeyko.com>,
	linux-btrfs@vger.kernel.org, David Sterba <dsterba@suse.com>,
	Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
	Nikolay Borisov <nborisov@suse.com>,
	Damien Le Moal <damien.lemoal@wdc.com>,
	Johannes Thumshirn <jthumshirn@suse.de>,
	Hannes Reinecke <hare@suse.com>,
	Anand Jain <anand.jain@oracle.com>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] libblkid: implement zone-aware probing for HMZONED btrfs
Date: Fri, 6 Dec 2019 16:22:39 +0100	[thread overview]
Message-ID: <20191206152238.GE2734@twin.jikos.cz> (raw)
In-Reply-To: <20191206070320.fzvqe4ketl3lx5q6@naota.dhcp.fujisawa.hgst.com>

On Fri, Dec 06, 2019 at 04:03:20PM +0900, Naohiro Aota wrote:
> >> +#define BTRFS_SUPER_INFO_SIZE 4096
> >
> >I believe that 4K is very widely used constant.
> >Are you sure that it needs to introduce some
> >additional constant? Especially, it looks slightly
> >strange to see the BTRFS specialized constant.
> >Maybe, it needs to generalize the constant?
> 
> I don't think so...
> 
> I think it is better to define BTRFS_SUPER_INFO_SIZE here. This is an
> already defined constant in btrfs-progs and this is key value to
> calculate the last superblock location. I think it's OK to define
> btrfs local constant in btrfs.c file...

I agree, the named constant makes the meaning more clear. In the code
where it's used:

> >> +	if (ret != -ENOENT) {
> >> +		if (wp == zones[0].start << SECTOR_SHIFT)
> >> +			wp = (zones[1].start + zones[1].len) <<
> >> SECTOR_SHIFT;
> >> +		wp -= BTRFS_SUPER_INFO_SIZE;
> >> +	}

If there's just

		wp -= 4096;

it's a magic constant out of nowhere. As pointed out, it's defined only
in btrfs.c so it does not pollute namespace in libblkid.