Re: [PATCH 2/2] btrfs-progs: read fsid from the sysfs in device_is_seed

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Oct 20, 2021 at 10:40:14AM +0800, Anand Jain wrote:
> 
> 
> On 19/10/2021 22:03, Josef Bacik wrote:
> > On Tue, Oct 19, 2021 at 08:23:45AM +0800, Anand Jain wrote:
> > > The kernel patch [1] added a sysfs interface to read the device fsid from
> > > the kernel, which is a better way to know the fsid of the device (rather
> > > than reading the superblock). It also works if in case the device is
> > > missing. Furthermore, the sysfs interface is readable from the non-root
> > > user.
> > > 
> > > So use this new sysfs interface here to read the fsid.
> > > 
> > > [1]
> > > btrfs: sysfs add devinfo/fsid to retrieve fsid from the device
> > > 
> > > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > > ---
> > >   cmds/filesystem-usage.c | 38 ++++++++++++++++++++++++++++++--------
> > >   1 file changed, 30 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> > > index 0dfc798e8dcc..f658c27b9609 100644
> > > --- a/cmds/filesystem-usage.c
> > > +++ b/cmds/filesystem-usage.c
> > > @@ -40,6 +40,7 @@
> > >   #include "common/help.h"
> > >   #include "common/device-utils.h"
> > >   #include "common/open-utils.h"
> > > +#include "common/path-utils.h"
> > >   /*
> > >    * Add the chunk info to the chunk_info list
> > > @@ -706,14 +707,33 @@ out:
> > >   	return ret;
> > >   }
> > > -static int device_is_seed(const char *dev_path, u8 *mnt_fsid)
> > > +static int device_is_seed(int fd, const char *dev_path, u64 devid, u8 *mnt_fsid)
> > >   {
> > > +	char fsidparse[BTRFS_UUID_UNPARSED_SIZE];
> > > +	char fsid_path[PATH_MAX];
> > > +	char devid_str[20];
> > >   	uuid_t fsid;
> > > -	int ret;
> > > +	int ret = -1;
> > > +	int sysfs_fd;
> > > +
> > > +	snprintf(devid_str, 20, "%llu", devid);
> > > +	/* devinfo/<devid>/fsid */
> > > +	path_cat3_out(fsid_path, "devinfo", devid_str, "fsid");
> > > +
> > > +	/* /sys/fs/btrfs/<fsid>/devinfo/<devid>/fsid */
> > > +	sysfs_fd = sysfs_open_fsid_file(fd, fsid_path);
> > > +	if (sysfs_fd >= 0) {
> > > +		sysfs_read_file(sysfs_fd, fsidparse, BTRFS_UUID_UNPARSED_SIZE);
> > > +		fsidparse[BTRFS_UUID_UNPARSED_SIZE - 1] = 0;
> > > +		ret = uuid_parse(fsidparse, fsid);
> > > +		close(sysfs_fd);
> > > +	}
> > > -	ret = dev_to_fsid(dev_path, fsid);
> > 
> > Why not just have dev_to_fsid() use the sysfs thing so all callers can benefit
> > from it, and then have it fall back to the reading of the super block?  Thanks,
> 
> If we are using sysfs to read fsid it means the device is mounted.
> 
> cmd_filesystem_show() uses dev_to_fsid() only if the device is unmounted. So
> we can't use fsid by sysfs here.
> 
> There is no other user of dev_to_fsid().
> 

Ah ok that's reasonable then, you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

to the series then.  Thanks,

Josef