| Message ID | 155259749135.31886.17642733861513368966.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | xfsprogs-5.0: fix various problems | expand |
On 3/14/19 4:04 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Back when I was designing xfs_scrub_all, I naïvely assumed that the > emitted output would always list physical storage before the virtual > devices stacked atop it. However, this is not actually true when one > omits the "NAME" column, I think I asked this on IRC but for the record, is this incidental or guaranteed by lsblk? I don't see this distinction in the manpage. I worry about depending on undocumented behavior... Looking back through IRC, I think we shrugged over this, I asked kzak but didn't get a reply, will try to find him again. (also FWIW, older lsblk rejects -J, do we care?) -Eric > which is crucial to forcing the output (json or > otherwise) to capture the block device hierarchy. If the assumption is > violated, the program crashes with a python exception. > > To fix this, force the hierarchal json output and restructure the > discovery routines to walk the json object that we receive, from the top > (physical devices) downwards to wherever there are live xfs filesystems. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > scrub/xfs_scrub_all.in | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > > diff --git a/scrub/xfs_scrub_all.in b/scrub/xfs_scrub_all.in > index c4e9899d..5b76b49a 100644 > --- a/scrub/xfs_scrub_all.in > +++ b/scrub/xfs_scrub_all.in > @@ -28,9 +28,21 @@ def DEVNULL(): > > def find_mounts(): > '''Map mountpoints to physical disks.''' > + def find_xfs_mounts(bdev, fs, lastdisk): > + '''Attach lastdisk to each fs found under bdev.''' > + if bdev['fstype'] == 'xfs' and bdev['mountpoint'] is not None: > + mnt = bdev['mountpoint'] > + if mnt in fs: > + fs[mnt].add(lastdisk) > + else: > + fs[mnt] = set([lastdisk]) > + if 'children' not in bdev: > + return > + for child in bdev['children']: > + find_xfs_mounts(child, fs, lastdisk) > > fs = {} > - cmd=['lsblk', '-o', 'KNAME,TYPE,FSTYPE,MOUNTPOINT', '-J'] > + cmd=['lsblk', '-o', 'NAME,KNAME,TYPE,FSTYPE,MOUNTPOINT', '-J'] > result = subprocess.Popen(cmd, stdout=subprocess.PIPE) > result.wait() > if result.returncode != 0: > @@ -38,18 +50,12 @@ def find_mounts(): > sarray = [x.decode(sys.stdout.encoding) for x in result.stdout.readlines()] > output = ' '.join(sarray) > bdevdata = json.loads(output) > + > # The lsblk output had better be in disks-then-partitions order > for bdev in bdevdata['blockdevices']: > - if bdev['type'] in ('disk', 'loop'): > - lastdisk = bdev['kname'] > - if bdev['fstype'] == 'xfs': > - mnt = bdev['mountpoint'] > - if mnt is None: > - continue > - if mnt in fs: > - fs[mnt].add(lastdisk) > - else: > - fs[mnt] = set([lastdisk]) > + lastdisk = bdev['kname'] > + find_xfs_mounts(bdev, fs, lastdisk) > + > return fs > > def kill_systemd(unit, proc): >
On Thu, Mar 14, 2019 at 09:46:26PM -0500, Eric Sandeen wrote: > On 3/14/19 4:04 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Back when I was designing xfs_scrub_all, I naïvely assumed that the > > emitted output would always list physical storage before the virtual > > devices stacked atop it. However, this is not actually true when one > > omits the "NAME" column, > > I think I asked this on IRC but for the record, is this incidental > or guaranteed by lsblk? I don't see this distinction in the manpage. Incidental. > I worry about depending on undocumented behavior... > > Looking back through IRC, I think we shrugged over this, I asked kzak > but didn't get a reply, will try to find him again. We did shrug about this. > (also FWIW, older lsblk rejects -J, do we care?) No, because xfs_scrub_all will just exit if lsblk returns nonzero. --D > -Eric > > > which is crucial to forcing the output (json or > > otherwise) to capture the block device hierarchy. If the assumption is > > violated, the program crashes with a python exception. > > > > To fix this, force the hierarchal json output and restructure the > > discovery routines to walk the json object that we receive, from the top > > (physical devices) downwards to wherever there are live xfs filesystems. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > scrub/xfs_scrub_all.in | 28 +++++++++++++++++----------- > > 1 file changed, 17 insertions(+), 11 deletions(-) > > > > > > diff --git a/scrub/xfs_scrub_all.in b/scrub/xfs_scrub_all.in > > index c4e9899d..5b76b49a 100644 > > --- a/scrub/xfs_scrub_all.in > > +++ b/scrub/xfs_scrub_all.in > > @@ -28,9 +28,21 @@ def DEVNULL(): > > > > def find_mounts(): > > '''Map mountpoints to physical disks.''' > > + def find_xfs_mounts(bdev, fs, lastdisk): > > + '''Attach lastdisk to each fs found under bdev.''' > > + if bdev['fstype'] == 'xfs' and bdev['mountpoint'] is not None: > > + mnt = bdev['mountpoint'] > > + if mnt in fs: > > + fs[mnt].add(lastdisk) > > + else: > > + fs[mnt] = set([lastdisk]) > > + if 'children' not in bdev: > > + return > > + for child in bdev['children']: > > + find_xfs_mounts(child, fs, lastdisk) > > > > fs = {} > > - cmd=['lsblk', '-o', 'KNAME,TYPE,FSTYPE,MOUNTPOINT', '-J'] > > + cmd=['lsblk', '-o', 'NAME,KNAME,TYPE,FSTYPE,MOUNTPOINT', '-J'] > > result = subprocess.Popen(cmd, stdout=subprocess.PIPE) > > result.wait() > > if result.returncode != 0: > > @@ -38,18 +50,12 @@ def find_mounts(): > > sarray = [x.decode(sys.stdout.encoding) for x in result.stdout.readlines()] > > output = ' '.join(sarray) > > bdevdata = json.loads(output) > > + > > # The lsblk output had better be in disks-then-partitions order > > for bdev in bdevdata['blockdevices']: > > - if bdev['type'] in ('disk', 'loop'): > > - lastdisk = bdev['kname'] > > - if bdev['fstype'] == 'xfs': > > - mnt = bdev['mountpoint'] > > - if mnt is None: > > - continue > > - if mnt in fs: > > - fs[mnt].add(lastdisk) > > - else: > > - fs[mnt] = set([lastdisk]) > > + lastdisk = bdev['kname'] > > + find_xfs_mounts(bdev, fs, lastdisk) > > + > > return fs > > > > def kill_systemd(unit, proc): > >
diff --git a/scrub/xfs_scrub_all.in b/scrub/xfs_scrub_all.in index c4e9899d..5b76b49a 100644 --- a/scrub/xfs_scrub_all.in +++ b/scrub/xfs_scrub_all.in @@ -28,9 +28,21 @@ def DEVNULL(): def find_mounts(): '''Map mountpoints to physical disks.''' + def find_xfs_mounts(bdev, fs, lastdisk): + '''Attach lastdisk to each fs found under bdev.''' + if bdev['fstype'] == 'xfs' and bdev['mountpoint'] is not None: + mnt = bdev['mountpoint'] + if mnt in fs: + fs[mnt].add(lastdisk) + else: + fs[mnt] = set([lastdisk]) + if 'children' not in bdev: + return + for child in bdev['children']: + find_xfs_mounts(child, fs, lastdisk) fs = {} - cmd=['lsblk', '-o', 'KNAME,TYPE,FSTYPE,MOUNTPOINT', '-J'] + cmd=['lsblk', '-o', 'NAME,KNAME,TYPE,FSTYPE,MOUNTPOINT', '-J'] result = subprocess.Popen(cmd, stdout=subprocess.PIPE) result.wait() if result.returncode != 0: @@ -38,18 +50,12 @@ def find_mounts(): sarray = [x.decode(sys.stdout.encoding) for x in result.stdout.readlines()] output = ' '.join(sarray) bdevdata = json.loads(output) + # The lsblk output had better be in disks-then-partitions order for bdev in bdevdata['blockdevices']: - if bdev['type'] in ('disk', 'loop'): - lastdisk = bdev['kname'] - if bdev['fstype'] == 'xfs': - mnt = bdev['mountpoint'] - if mnt is None: - continue - if mnt in fs: - fs[mnt].add(lastdisk) - else: - fs[mnt] = set([lastdisk]) + lastdisk = bdev['kname'] + find_xfs_mounts(bdev, fs, lastdisk) + return fs def kill_systemd(unit, proc):