[GRUB] xfs: accept filesystem with sparse inodes
diff mbox

Message ID ce584ae1-ee62-2dcb-9366-eb0a6df6e98e@sandeen.net
State New
Headers show

Commit Message

Eric Sandeen May 15, 2018, 7:55 p.m. UTC
The sparse inode metadata format became a mkfs.xfs default in
xfsprogs-4.16.0, and such filesystems are now rejected by grub as
containing an incompatible feature.

In essence, this feature allows xfs to allocate inodes into fragmented
freespace.  (Without this feature, if xfs could not allocate contiguous
space for 64 new inodes, inode creation would fail.)

In practice, the disk format change is restricted to the inode btree,
which as far as I can tell is not used by grub.  If all you're doing
today is parsing a directory, reading an inode number, and converting
that inode number to a disk location, then ignoring this feature
should be fine, so I've added it to XFS_SB_FEAT_INCOMPAT_SUPPORTED

I did some brief testing of this patch by hacking up the regression
tests to completely fragment freespace on the test xfs filesystem, and
then write a large-ish number of inodes to consume any existing
contiguous 64-inode chunk.  This way any files the grub tests add and
traverse would be in such a fragmented inode allocation.  Tests passed,
but I'm not sure how to cleanly integrate that into the test harness.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---


--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Daniel Kiper May 16, 2018, 9 a.m. UTC | #1
On Tue, May 15, 2018 at 02:55:55PM -0500, Eric Sandeen wrote:
> The sparse inode metadata format became a mkfs.xfs default in
> xfsprogs-4.16.0, and such filesystems are now rejected by grub as
> containing an incompatible feature.
>
> In essence, this feature allows xfs to allocate inodes into fragmented
> freespace.  (Without this feature, if xfs could not allocate contiguous
> space for 64 new inodes, inode creation would fail.)
>
> In practice, the disk format change is restricted to the inode btree,
> which as far as I can tell is not used by grub.  If all you're doing
> today is parsing a directory, reading an inode number, and converting
> that inode number to a disk location, then ignoring this feature
> should be fine, so I've added it to XFS_SB_FEAT_INCOMPAT_SUPPORTED
>
> I did some brief testing of this patch by hacking up the regression
> tests to completely fragment freespace on the test xfs filesystem, and
> then write a large-ish number of inodes to consume any existing
> contiguous 64-inode chunk.  This way any files the grub tests add and
> traverse would be in such a fragmented inode allocation.  Tests passed,
> but I'm not sure how to cleanly integrate that into the test harness.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Eric, thank you for posting the patch. LGTM.

Chris, may I ask you to test it and add your "Tested-by:" if it works?

Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Murphy May 19, 2018, 5:52 p.m. UTC | #2
On Wed, May 16, 2018 at 3:00 AM, Daniel Kiper <dkiper@net-space.pl> wrote:
> On Tue, May 15, 2018 at 02:55:55PM -0500, Eric Sandeen wrote:
>> The sparse inode metadata format became a mkfs.xfs default in
>> xfsprogs-4.16.0, and such filesystems are now rejected by grub as
>> containing an incompatible feature.
>>
>> In essence, this feature allows xfs to allocate inodes into fragmented
>> freespace.  (Without this feature, if xfs could not allocate contiguous
>> space for 64 new inodes, inode creation would fail.)
>>
>> In practice, the disk format change is restricted to the inode btree,
>> which as far as I can tell is not used by grub.  If all you're doing
>> today is parsing a directory, reading an inode number, and converting
>> that inode number to a disk location, then ignoring this feature
>> should be fine, so I've added it to XFS_SB_FEAT_INCOMPAT_SUPPORTED
>>
>> I did some brief testing of this patch by hacking up the regression
>> tests to completely fragment freespace on the test xfs filesystem, and
>> then write a large-ish number of inodes to consume any existing
>> contiguous 64-inode chunk.  This way any files the grub tests add and
>> traverse would be in such a fragmented inode allocation.  Tests passed,
>> but I'm not sure how to cleanly integrate that into the test harness.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>
> Eric, thank you for posting the patch. LGTM.
>
> Chris, may I ask you to test it and add your "Tested-by:" if it works?

Fedora openQA (which caught the bug) now passes. I did a separate test
making sure sparse inode feature is enabled and grub-probe,
grub-install, grub-mkconfig report no problem. So yeah feel free to
Tested-by to me.

Patch
diff mbox

diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
index c6031bd3f..effe9de17 100644
--- a/grub-core/fs/xfs.c
+++ b/grub-core/fs/xfs.c
@@ -79,9 +79,18 @@  GRUB_MOD_LICENSE ("GPLv3+");
  #define XFS_SB_FEAT_INCOMPAT_SPINODES   (1 << 1)        /* sparse inode chunks */
  #define XFS_SB_FEAT_INCOMPAT_META_UUID  (1 << 2)        /* metadata UUID */
  
-/* We do not currently verify metadata UUID so it is safe to read such filesystem */
+/*
+ * Directory entries with ftype are explicitly handled by grub code.
+ *
+ * We do not currently verify metadata UUID, so it is safe to read filesystems
+ * with the XFS_SB_FEAT_INCOMPAT_META_UUID feature.
+ *
+ * We do not currently read the inode btrees, so it is safe to read filesystems
+ * with the XFS_SB_FEAT_INCOMPAT_SPINODES feature.
+ */
  #define XFS_SB_FEAT_INCOMPAT_SUPPORTED \
  	(XFS_SB_FEAT_INCOMPAT_FTYPE | \
+	 XFS_SB_FEAT_INCOMPAT_SPINODES | \
  	 XFS_SB_FEAT_INCOMPAT_META_UUID)
  
  struct grub_xfs_sblock