diff mbox series

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

Message ID 154524777080.28646.13859873012142786308.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfsprogs: various fixes | expand

Commit Message

Darrick J. Wong Dec. 19, 2018, 7:29 p.m. UTC 
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.

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(-)

Comments

Eric Sandeen Feb. 4, 2019, 6:08 p.m. UTC | #1 
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?
 
> 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?

-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):
>
Darrick J. Wong Feb. 4, 2019, 6:16 p.m. UTC | #2 
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):
> >
Eric Sandeen Feb. 4, 2019, 6:27 p.m. UTC | #3 
On 2/4/19 12:16 PM, Darrick J. Wong wrote:
> 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?

bleah.  Sounds fragile.  :(

Let me check with kzak on this.  If your patch improves it for now, super,
but I don't want to leave a little time bomb here.

>>> -	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.)

Yeah I'd expect that as well, just wondered how much of a landmine it
might be, I guess.

-Eric

> --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):
>>>
>
diff mbox series

Patch

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