| Message ID | 873d173c3b16fcd027dba4b10690e3e3fc3b6cdd.1634598659.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: read device fsid from the sysfs | expand |
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, Josef
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(). Thanks, Anand > > Josef >
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
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); - if (ret) - return ret; + if (ret) { + ret = dev_to_fsid(dev_path, fsid); + if (ret) + return ret; + } if (memcmp(mnt_fsid, fsid, BTRFS_FSID_SIZE)) return 0; @@ -768,13 +788,15 @@ static int load_device_info(int fd, struct device_info **device_info_ptr, } /* - * Skip seed device by checking device's fsid (requires root). - * And we will skip only if dev_to_fsid is successful and dev + * Skip seed device by checking device's fsid (requires root if + * kernel is not patched to provide fsid from the sysfs). + * And we will skip only if device_is_seed is successful and dev * is a seed device. * Ignore any other error including -EACCES, which is seen when * a non-root process calls dev_to_fsid(path)->open(path). */ - ret = device_is_seed((const char *)dev_info.path, fi_args.fsid); + ret = device_is_seed(fd, (const char *)dev_info.path, i, + fi_args.fsid); if (!ret) continue;
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(-)