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