Re: xfs_admin -O feature upgrade feedback

Re: xfs_admin -O feature upgrade feedback

[Darrick J. Wong]

[adding linux-xfs to cc]

On Mon, Mar 01, 2021 at 11:13:14AM +0100, Geert Hendrickx wrote:
> Gentlemen
> 
> 
> I've been testing xfsprogs 5.11.0-rc1 for XFS feature upgrades (inobtcount
> and bigtime), and have a couple of small nits with it - besides the actual
> functionality working as expected:
> 
> 1/ xfs_admin responds to every xfs_repair failure with the very generic
> "Conversion failed, is the filesystem unmounted?"  This isn't very helpful
> (and left me scratching my head in a number of scenarios), whereas calling
> xfs_repair directly shows a relevant error message in all cases.  This
> output should somehow be relayed through xfs_admin - without just dumping
> the whole xfs_repair output which I know you wanted to avoid.  Maybe by
> distinguishing more carefully between stderr and stdout?  (Currently, it
> seems xfs_repair sends its errors to stdout and "normal output" to stderr,
> and xfs_admin discards xfs_repair's stderr.)

That's a difficult project -- some of the things repair will complain
about are a result of whatever the upgrade is (e.g. complaining about
incorrect inode btree counters when you're in the process of enabling
the counters) but then there are other things that it probably should
not be dropping on the floor.

> 2/ minor, but xfs_admin(8) manpage documents `xfs_admin -O feature=status`,
> however xfs_admin itself appends another `=1`, resulting in `xfs_repair -c
> feature=1=1`.  This works, but looks ugly, and is not consistent with the
> option to enable multiple features at once.  I think either the xfs_admin
> script or its manpage should be adjusted to be consistent?

Yeah, xfs_admin should not add '=1'. Thanks for pointing that out.

> Apart from this, the actual upgrade functionality is working as expected,
> great job!

Thx :)

> Btw, do you have an idea from which release onwards mkfs.xfs will enable
> the new features by default?  Are there fixed rules for this (like feature
> must be X releases old, or supported by the latest LTS kernel), or is this
> judged on a case-by-case basis?

~6mo after we hear that people are using the feature /and/ we haven't
heard of any serious complaints.

Or some distro makes a business case and enables it by default. <cough>

--D

> 
> Thanks!
> 
> 
> 	Geert
> 
> 

Re: xfs_admin -O feature upgrade feedback

[Geert Hendrickx]

On Mon, Mar 01, 2021 at 11:18:03 -0800, Darrick J. Wong wrote:
> On Mon, Mar 01, 2021 at 11:13:14AM +0100, Geert Hendrickx wrote:
> > 1/ xfs_admin responds to every xfs_repair failure with the very generic
> > "Conversion failed, is the filesystem unmounted?"  This isn't very helpful
> > (and left me scratching my head in a number of scenarios), whereas calling
> > xfs_repair directly shows a relevant error message in all cases.  This
> > output should somehow be relayed through xfs_admin - without just dumping
> > the whole xfs_repair output which I know you wanted to avoid.  Maybe by
> > distinguishing more carefully between stderr and stdout?  (Currently, it
> > seems xfs_repair sends its errors to stdout and "normal output" to stderr,
> > and xfs_admin discards xfs_repair's stderr.)
> 
> That's a difficult project -- some of the things repair will complain
> about are a result of whatever the upgrade is (e.g. complaining about
> incorrect inode btree counters when you're in the process of enabling
> the counters) but then there are other things that it probably should
> not be dropping on the floor.



On the other hand, upgrading a filesystem is a one-time operation where
it doesn't hurt to be a bit more verbose by default, compared to regular
maintenance operations where silence indicates success.

(and for large filesystems some progress indication is useful, too.)


	Geert

Re: xfs_admin -O feature upgrade feedback

[Brian Foster]

On Mon, Mar 01, 2021 at 11:18:03AM -0800, Darrick J. Wong wrote:
> [adding linux-xfs to cc]
> 
> On Mon, Mar 01, 2021 at 11:13:14AM +0100, Geert Hendrickx wrote:
> > Gentlemen
> > 
> > 
> > I've been testing xfsprogs 5.11.0-rc1 for XFS feature upgrades (inobtcount
> > and bigtime), and have a couple of small nits with it - besides the actual
> > functionality working as expected:
> > 
> > 1/ xfs_admin responds to every xfs_repair failure with the very generic
> > "Conversion failed, is the filesystem unmounted?"  This isn't very helpful
> > (and left me scratching my head in a number of scenarios), whereas calling
> > xfs_repair directly shows a relevant error message in all cases.  This
> > output should somehow be relayed through xfs_admin - without just dumping
> > the whole xfs_repair output which I know you wanted to avoid.  Maybe by
> > distinguishing more carefully between stderr and stdout?  (Currently, it
> > seems xfs_repair sends its errors to stdout and "normal output" to stderr,
> > and xfs_admin discards xfs_repair's stderr.)
> 
> That's a difficult project -- some of the things repair will complain
> about are a result of whatever the upgrade is (e.g. complaining about
> incorrect inode btree counters when you're in the process of enabling
> the counters) but then there are other things that it probably should
> not be dropping on the floor.
> 

It's not clear to me if you're reporting that feature upgrades
spuriously report this "Conversion failed ..." message (i.e., feature
upgrade succeeded, but repair found and fixed things expected to be
problems due to the feature upgrade), or that this error is reported if
there is something independently wrong with the fs. If the former, that
seems like a bug. If the latter, I think that's reasonable/expected
behavior.

IMO, the fact that the feature upgrade runs xfs_repair is an
implementation detail. There's no guarantee that repair might always be
necessary for this operation or that it would find/fix other issues when
running for the purpose of a feature upgrade. For that reason, I don't
think it makes a whole lot of sense to try and pipe detailed repair
messages through xfs_admin (as opposed to generically informative
messages like "upgrade succeeded," "upgrade failed," "filesystem
corrupt?" etc). Just my .02, of course.

Brian

> > 2/ minor, but xfs_admin(8) manpage documents `xfs_admin -O feature=status`,
> > however xfs_admin itself appends another `=1`, resulting in `xfs_repair -c
> > feature=1=1`.  This works, but looks ugly, and is not consistent with the
> > option to enable multiple features at once.  I think either the xfs_admin
> > script or its manpage should be adjusted to be consistent?
> 
> Yeah, xfs_admin should not add '=1'. Thanks for pointing that out.
> 
> > Apart from this, the actual upgrade functionality is working as expected,
> > great job!
> 
> Thx :)
> 
> > Btw, do you have an idea from which release onwards mkfs.xfs will enable
> > the new features by default?  Are there fixed rules for this (like feature
> > must be X releases old, or supported by the latest LTS kernel), or is this
> > judged on a case-by-case basis?
> 
> ~6mo after we hear that people are using the feature /and/ we haven't
> heard of any serious complaints.
> 
> Or some distro makes a business case and enables it by default. <cough>
> 
> --D
> 
> > 
> > Thanks!
> > 
> > 
> > 	Geert
> > 
> > 
> 

Re: xfs_admin -O feature upgrade feedback

[Geert Hendrickx]

On Tue, Mar 02, 2021 at 07:19:37 -0500, Brian Foster wrote:
> It's not clear to me if you're reporting that feature upgrades spuriously
> report this "Conversion failed ..." message (i.e., feature upgrade
> succeeded, but repair found and fixed things expected to be problems due
> to the feature upgrade), or that this error is reported if there is
> something independently wrong with the fs. If the former, that seems like
> a bug. If the latter, I think that's reasonable/expected behavior.



There are sillier scenarios, like simply incorrect arguments.  For example
"xfs_admin -O bigtypo=1 /dev/foo" responds with: "Conversion failed, is the
filesystem unmounted?"

(where /dev/foo is the correct blockdevice, properly unmounted etc, but the
options argument contains a typo)

The proper xfs_repair error "unknown option -c bigtypo=1" gets thrown away.


Other examples include "-O bigtime" => "bigtime requires a parameter" (with
Darrick's patch for the other issue applied), or "bigtime=0" => "bigtime
only supports upgrades", all dropped on the floor by xfs_admin and replaced
with the one generic message that gives no indication of the actual problem.
(the user keeps verifying whether the filesystem is unmounted and clean...)



	Geert

Re: xfs_admin -O feature upgrade feedback

[Brian Foster]

On Tue, Mar 02, 2021 at 11:57:22PM +0100, Geert Hendrickx wrote:
> On Tue, Mar 02, 2021 at 07:19:37 -0500, Brian Foster wrote:
> > It's not clear to me if you're reporting that feature upgrades spuriously
> > report this "Conversion failed ..." message (i.e., feature upgrade
> > succeeded, but repair found and fixed things expected to be problems due
> > to the feature upgrade), or that this error is reported if there is
> > something independently wrong with the fs. If the former, that seems like
> > a bug. If the latter, I think that's reasonable/expected behavior.
> 
> 
> 
> There are sillier scenarios, like simply incorrect arguments.  For example
> "xfs_admin -O bigtypo=1 /dev/foo" responds with: "Conversion failed, is the
> filesystem unmounted?"
> 
> (where /dev/foo is the correct blockdevice, properly unmounted etc, but the
> options argument contains a typo)
> 
> The proper xfs_repair error "unknown option -c bigtypo=1" gets thrown away.
> 
> 
> Other examples include "-O bigtime" => "bigtime requires a parameter" (with
> Darrick's patch for the other issue applied), or "bigtime=0" => "bigtime
> only supports upgrades", all dropped on the floor by xfs_admin and replaced
> with the one generic message that gives no indication of the actual problem.
> (the user keeps verifying whether the filesystem is unmounted and clean...)
> 

Ok. I suppose in the scenario where xfs_repair runs on behalf of
xfs_admin and then fails immediately due to a usage error, it might be
more appropriate to dump whatever error xfs_repair exits with. I'm not
sure how best to filter that and/or deal with the issues Darrick points
out, but fair point...

Maybe a simple compromise is a verbose option for xfs_admin itself..?
I.e., the normal use case operates as it does now, but the failure case
would print something like:

  "Feature conversion failed. Retry with -v for detailed error output."

... and then 'xfs_admin -v ...' would just pass through xfs_repair
output. Eh?

Brian

> 
> 
> 	Geert
> 
> 

Re: xfs_admin -O feature upgrade feedback

[Geert Hendrickx]

On Wed, Mar 03, 2021 at 06:50:04 -0500, Brian Foster wrote:
> Maybe a simple compromise is a verbose option for xfs_admin itself..?
> I.e., the normal use case operates as it does now, but the failure case
> would print something like:
> 
>   "Feature conversion failed. Retry with -v for detailed error output."
> 
> ... and then 'xfs_admin -v ...' would just pass through xfs_repair
> output. Eh?


Good suggestion, that should cover it.


	Geert

Re: xfs_admin -O feature upgrade feedback

[Darrick J. Wong]

On Wed, Mar 03, 2021 at 02:20:23PM +0100, Geert Hendrickx wrote:
> On Wed, Mar 03, 2021 at 06:50:04 -0500, Brian Foster wrote:
> > Maybe a simple compromise is a verbose option for xfs_admin itself..?
> > I.e., the normal use case operates as it does now, but the failure case
> > would print something like:
> > 
> >   "Feature conversion failed. Retry with -v for detailed error output."

Ugh, no, by the time the sysadmin /reruns/ repair, the original output
is lost.  Frankly I'd rather xfs_admin stop interfering with
stdout/stderr and teach repair to suppress errors due to upgrades.

--D

> > ... and then 'xfs_admin -v ...' would just pass through xfs_repair
> > output. Eh?
> 
> 
> Good suggestion, that should cover it.
> 
> 
> 	Geert
> 
> 

Re: xfs_admin -O feature upgrade feedback

[Dave Chinner]

On Wed, Mar 03, 2021 at 09:00:19AM -0800, Darrick J. Wong wrote:
> On Wed, Mar 03, 2021 at 02:20:23PM +0100, Geert Hendrickx wrote:
> > On Wed, Mar 03, 2021 at 06:50:04 -0500, Brian Foster wrote:
> > > Maybe a simple compromise is a verbose option for xfs_admin itself..?
> > > I.e., the normal use case operates as it does now, but the failure case
> > > would print something like:
> > > 
> > >   "Feature conversion failed. Retry with -v for detailed error output."
> 
> Ugh, no, by the time the sysadmin /reruns/ repair, the original output
> is lost.  Frankly I'd rather xfs_admin stop interfering with
> stdout/stderr and teach repair to suppress errors due to upgrades.

Yup, that's what the 'xfs_db -p <progname>' effectively does. It
tells xfs_db to act as if it was some other program and behave
differently....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com