Re: [PATCH 1/8] xfs_scrub_all: walk the lsblk device/fs hierarchy correctly

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Mon, Feb 04, 2019 at 12:08:49PM -0600, Eric Sandeen wrote:
> On 12/19/18 1:29 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, 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.
> 
> Is this a quirk or a documented feature of lsblk?

Not a documented feature, but seems to be a fairly common behavioral
quirk?

> > 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']
> 
> sorry for the ridonculously late review, and although '-J" isn't added
> new in this patch, FYI at least RHEL7 does not allow it:
> 
> # lsblk -o KNAME -J
> lsblk: invalid option -- 'J'
> 
> ... thoughts?  Probably should be handled gracefully at least?

lsblk returns 1 for unrecognized arguments, so xfs_scrub_all will bail
if lsblk barfs.  Not sure if we want to divert lsblk's stderr to
/dev/null or just let it spray out sloppily like we do now?

(Practically speaking I suspect that distros will pick up the util-linux
release that has json support before they pick up XFS kernel scrub...
but I should at least make sure that's really true.)

--D

> -Eric
> 
> >  	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):
> >