xfs: truncate transaction does not modify the inobt

Commit Message

Amir Goldstein Nov. 3, 2018, 5:15 p.m. UTC

From: Brian Foster <[e-mail]>

The truncate transaction does not ever modify the inode btree, but
includes an associated log reservation. Update
xfs_calc_itruncate_reservation() to remove the reservation
associated with inobt updates.

[Amir:	This commit was merged for kernel v4.16 and a twin commit was
	merged for xfsprogs v4.16. As a result, a small xfs filesystem
	formatted with features -m rmapbt=1,reflink=1 using mkfs.xfs
	version >= v4.16 cannot be mounted with kernel < v4.16.

	For example, xfstests generic/17{1,2,3} format a small fs and
	when trying to mount it, they fail with an assert on this very
	demonic line:

 XFS (vdc): Log size 3075 blocks too small, minimum size is 3717 blocks
 XFS (vdc): AAIEEE! Log failed size checks. Abort!
 XFS: Assertion failed: 0, file: src/linux/fs/xfs/xfs_log.c, line: 666

	The simple solution for stable kernels is to apply this patch,
	because mkfs.xfs v4.16 is already in the wild, so we have to
	assume that xfs filesystems with a "too small" log exist.
	Regardless, xfsprogs maintainers should also consider reverting
	the twin patch to stop creating those filesystems for the sake
	of users with unpatched kernels.]

Signed-off-by: Brian Foster <[e-mail]>
Reviewed-by: Dave Chinner <[e-mail]>
Reviewed-by: Darrick J. Wong <[e-mail]>
Signed-off-by: Darrick J. Wong <[e-mail]>
Cc: <[e-mail]> # v4.9+
Signed-off-by: Amir Goldstein <[e-mail]>
---

Darrick/Dave,

It took me a while to figure out what was going on with my test systems
when small test partitions (10G) stopped working with older kernels.

Please bless this change for stable and consider the remedie for mkfs.xfs
I verified that patch cleanly applies to stable kernels 4.14.y and 4.9.y
and that I can mount a filsystem created with new mkfs.xfs.

I am now running quick tests on stable 4.14.y with configs 4k, 1k,
reflink,reflink+overlay to verify no regressions from this patch.

Thanks,
Amir.


 fs/xfs/libxfs/xfs_trans_resv.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

Amir Goldstein Nov. 5, 2018, 5:57 p.m. UTC | #1

On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <[e-mail]> wrote:
>
> From: Brian Foster <[e-mail]>
>
> The truncate transaction does not ever modify the inode btree, but
> includes an associated log reservation. Update
> xfs_calc_itruncate_reservation() to remove the reservation
> associated with inobt updates.
>
> [Amir:  This commit was merged for kernel v4.16 and a twin commit was
>         merged for xfsprogs v4.16. As a result, a small xfs filesystem
>         formatted with features -m rmapbt=1,reflink=1 using mkfs.xfs
>         version >= v4.16 cannot be mounted with kernel < v4.16.
>
>         For example, xfstests generic/17{1,2,3} format a small fs and
>         when trying to mount it, they fail with an assert on this very
>         demonic line:
>
>  XFS (vdc): Log size 3075 blocks too small, minimum size is 3717 blocks
>  XFS (vdc): AAIEEE! Log failed size checks. Abort!
>  XFS: Assertion failed: 0, file: src/linux/fs/xfs/xfs_log.c, line: 666
>
>         The simple solution for stable kernels is to apply this patch,
>         because mkfs.xfs v4.16 is already in the wild, so we have to
>         assume that xfs filesystems with a "too small" log exist.
>         Regardless, xfsprogs maintainers should also consider reverting
>         the twin patch to stop creating those filesystems for the sake
>         of users with unpatched kernels.]
>
> Signed-off-by: Brian Foster <[e-mail]>
> Reviewed-by: Dave Chinner <[e-mail]>
> Reviewed-by: Darrick J. Wong <[e-mail]>
> Signed-off-by: Darrick J. Wong <[e-mail]>
> Cc: <[e-mail]> # v4.9+
> Signed-off-by: Amir Goldstein <[e-mail]>
> ---
>
> Darrick/Dave,
>
> It took me a while to figure out what was going on with my test systems
> when small test partitions (10G) stopped working with older kernels.
>
> Please bless this change for stable and consider the remedie for mkfs.xfs
> I verified that patch cleanly applies to stable kernels 4.14.y and 4.9.y
> and that I can mount a filsystem created with new mkfs.xfs.
>
> I am now running quick tests on stable 4.14.y with configs 4k, 1k,
> reflink,reflink+overlay to verify no regressions from this patch.
>

FYI no regressions detected.

Thoughts?

Thanks,
Amir.

>
>
>  fs/xfs/libxfs/xfs_trans_resv.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 6bd916bd35e2..48eff18c5496 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -232,8 +232,6 @@ xfs_calc_write_reservation(
>   *    the super block to reflect the freed blocks: sector size
>   *    worst case split in allocation btrees per extent assuming 4 extents:
>   *             4 exts * 2 trees * (2 * max depth - 1) * block size
> - *    the inode btree: max depth * blocksize
> - *    the allocation btrees: 2 trees * (max depth - 1) * block size
>   */
>  STATIC uint
>  xfs_calc_itruncate_reservation(
> @@ -245,12 +243,7 @@ xfs_calc_itruncate_reservation(
>                                       XFS_FSB_TO_B(mp, 1))),
>                     (xfs_calc_buf_res(9, mp->m_sb.sb_sectsize) +
>                      xfs_calc_buf_res(xfs_allocfree_log_count(mp, 4),
> -                                     XFS_FSB_TO_B(mp, 1)) +
> -                   xfs_calc_buf_res(5, 0) +
> -                   xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -                                    XFS_FSB_TO_B(mp, 1)) +
> -                   xfs_calc_buf_res(2 + mp->m_ialloc_blks +
> -                                    mp->m_in_maxlevels, 0)));
> +                                     XFS_FSB_TO_B(mp, 1))));
>  }
>
>  /*
> --
> 2.17.1
>

Amir Goldstein Nov. 7, 2018, 5:09 a.m. UTC | #2

On Mon, Nov 5, 2018 at 7:57 PM Amir Goldstein <[e-mail]> wrote:
>
> On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <[e-mail]> wrote:
> >
> > From: Brian Foster <[e-mail]>
> >
> > The truncate transaction does not ever modify the inode btree, but
> > includes an associated log reservation. Update
> > xfs_calc_itruncate_reservation() to remove the reservation
> > associated with inobt updates.
> >
> > [Amir:  This commit was merged for kernel v4.16 and a twin commit was
> >         merged for xfsprogs v4.16. As a result, a small xfs filesystem
> >         formatted with features -m rmapbt=1,reflink=1 using mkfs.xfs
> >         version >= v4.16 cannot be mounted with kernel < v4.16.
> >
> >         For example, xfstests generic/17{1,2,3} format a small fs and
> >         when trying to mount it, they fail with an assert on this very
> >         demonic line:
> >
> >  XFS (vdc): Log size 3075 blocks too small, minimum size is 3717 blocks
> >  XFS (vdc): AAIEEE! Log failed size checks. Abort!
> >  XFS: Assertion failed: 0, file: src/linux/fs/xfs/xfs_log.c, line: 666
> >
> >         The simple solution for stable kernels is to apply this patch,
> >         because mkfs.xfs v4.16 is already in the wild, so we have to
> >         assume that xfs filesystems with a "too small" log exist.
> >         Regardless, xfsprogs maintainers should also consider reverting
> >         the twin patch to stop creating those filesystems for the sake
> >         of users with unpatched kernels.]
> >
> > Signed-off-by: Brian Foster <[e-mail]>
> > Reviewed-by: Dave Chinner <[e-mail]>
> > Reviewed-by: Darrick J. Wong <[e-mail]>
> > Signed-off-by: Darrick J. Wong <[e-mail]>
> > Cc: <[e-mail]> # v4.9+
> > Signed-off-by: Amir Goldstein <[e-mail]>
> > ---
> >
> > Darrick/Dave,
> >
> > It took me a while to figure out what was going on with my test systems
> > when small test partitions (10G) stopped working with older kernels.
> >
> > Please bless this change for stable and consider the remedie for mkfs.xfs
> > I verified that patch cleanly applies to stable kernels 4.14.y and 4.9.y
> > and that I can mount a filsystem created with new mkfs.xfs.
> >
> > I am now running quick tests on stable 4.14.y with configs 4k, 1k,
> > reflink,reflink+overlay to verify no regressions from this patch.
> >
>
> FYI no regressions detected.
>
> Thoughts?
>

Maybe you'd want to chalk it up to reflink/rmapbt being Experimental
before kernel 4.16? so the change in "minimum log size" is an on-disk format
change prior to removing the Experimental label??

Thanks,
Amir.

Darrick J. Wong Nov. 7, 2018, 5:18 a.m. UTC | #3

On Wed, Nov 07, 2018 at 07:09:42AM +0200, Amir Goldstein wrote:
> On Mon, Nov 5, 2018 at 7:57 PM Amir Goldstein <[e-mail]> wrote:
> >
> > On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <[e-mail]> wrote:
> > >
> > > From: Brian Foster <[e-mail]>
> > >
> > > The truncate transaction does not ever modify the inode btree, but
> > > includes an associated log reservation. Update
> > > xfs_calc_itruncate_reservation() to remove the reservation
> > > associated with inobt updates.
> > >
> > > [Amir:  This commit was merged for kernel v4.16 and a twin commit was
> > >         merged for xfsprogs v4.16. As a result, a small xfs filesystem
> > >         formatted with features -m rmapbt=1,reflink=1 using mkfs.xfs
> > >         version >= v4.16 cannot be mounted with kernel < v4.16.
> > >
> > >         For example, xfstests generic/17{1,2,3} format a small fs and
> > >         when trying to mount it, they fail with an assert on this very
> > >         demonic line:
> > >
> > >  XFS (vdc): Log size 3075 blocks too small, minimum size is 3717 blocks
> > >  XFS (vdc): AAIEEE! Log failed size checks. Abort!
> > >  XFS: Assertion failed: 0, file: src/linux/fs/xfs/xfs_log.c, line: 666
> > >
> > >         The simple solution for stable kernels is to apply this patch,
> > >         because mkfs.xfs v4.16 is already in the wild, so we have to
> > >         assume that xfs filesystems with a "too small" log exist.
> > >         Regardless, xfsprogs maintainers should also consider reverting
> > >         the twin patch to stop creating those filesystems for the sake
> > >         of users with unpatched kernels.]
> > >
> > > Signed-off-by: Brian Foster <[e-mail]>
> > > Reviewed-by: Dave Chinner <[e-mail]>
> > > Reviewed-by: Darrick J. Wong <[e-mail]>
> > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > Cc: <[e-mail]> # v4.9+
> > > Signed-off-by: Amir Goldstein <[e-mail]>
> > > ---
> > >
> > > Darrick/Dave,
> > >
> > > It took me a while to figure out what was going on with my test systems
> > > when small test partitions (10G) stopped working with older kernels.
> > >
> > > Please bless this change for stable and consider the remedie for mkfs.xfs
> > > I verified that patch cleanly applies to stable kernels 4.14.y and 4.9.y
> > > and that I can mount a filsystem created with new mkfs.xfs.
> > >
> > > I am now running quick tests on stable 4.14.y with configs 4k, 1k,
> > > reflink,reflink+overlay to verify no regressions from this patch.
> > >
> >
> > FYI no regressions detected.
> >
> > Thoughts?
> >
> 
> Maybe you'd want to chalk it up to reflink/rmapbt being Experimental
> before kernel 4.16? so the change in "minimum log size" is an on-disk format
> change prior to removing the Experimental label??

TBH nobody should be using reflink/rmap on 4.14 kernels, ever. :D

That said... does it change the minimum log size for (finobt, !reflink,
!rmap) filesystems?  That might be a bigger worry.  I /think/ the
transaction reservation change is fine, though I defer to Amir on
testing... :)

--D

> Thanks,
> Amir.

Amir Goldstein Nov. 7, 2018, 5:31 a.m. UTC | #4

On Wed, Nov 7, 2018 at 7:18 AM Darrick J. Wong <[e-mail]> wrote:
>
> On Wed, Nov 07, 2018 at 07:09:42AM +0200, Amir Goldstein wrote:
> > On Mon, Nov 5, 2018 at 7:57 PM Amir Goldstein <[e-mail]> wrote:
> > >
> > > On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <[e-mail]> wrote:
> > > >
> > > > From: Brian Foster <[e-mail]>
> > > >
> > > > The truncate transaction does not ever modify the inode btree, but
> > > > includes an associated log reservation. Update
> > > > xfs_calc_itruncate_reservation() to remove the reservation
> > > > associated with inobt updates.
> > > >
> > > > [Amir:  This commit was merged for kernel v4.16 and a twin commit was
> > > >         merged for xfsprogs v4.16. As a result, a small xfs filesystem
> > > >         formatted with features -m rmapbt=1,reflink=1 using mkfs.xfs
> > > >         version >= v4.16 cannot be mounted with kernel < v4.16.
> > > >
> > > >         For example, xfstests generic/17{1,2,3} format a small fs and
> > > >         when trying to mount it, they fail with an assert on this very
> > > >         demonic line:
> > > >
> > > >  XFS (vdc): Log size 3075 blocks too small, minimum size is 3717 blocks
> > > >  XFS (vdc): AAIEEE! Log failed size checks. Abort!
> > > >  XFS: Assertion failed: 0, file: src/linux/fs/xfs/xfs_log.c, line: 666
> > > >
> > > >         The simple solution for stable kernels is to apply this patch,
> > > >         because mkfs.xfs v4.16 is already in the wild, so we have to
> > > >         assume that xfs filesystems with a "too small" log exist.
> > > >         Regardless, xfsprogs maintainers should also consider reverting
> > > >         the twin patch to stop creating those filesystems for the sake
> > > >         of users with unpatched kernels.]
> > > >
> > > > Signed-off-by: Brian Foster <[e-mail]>
> > > > Reviewed-by: Dave Chinner <[e-mail]>
> > > > Reviewed-by: Darrick J. Wong <[e-mail]>
> > > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > > Cc: <[e-mail]> # v4.9+
> > > > Signed-off-by: Amir Goldstein <[e-mail]>
> > > > ---
> > > >
> > > > Darrick/Dave,
> > > >
> > > > It took me a while to figure out what was going on with my test systems
> > > > when small test partitions (10G) stopped working with older kernels.
> > > >
> > > > Please bless this change for stable and consider the remedie for mkfs.xfs
> > > > I verified that patch cleanly applies to stable kernels 4.14.y and 4.9.y
> > > > and that I can mount a filsystem created with new mkfs.xfs.
> > > >
> > > > I am now running quick tests on stable 4.14.y with configs 4k, 1k,
> > > > reflink,reflink+overlay to verify no regressions from this patch.
> > > >
> > >
> > > FYI no regressions detected.
> > >
> > > Thoughts?
> > >
> >
> > Maybe you'd want to chalk it up to reflink/rmapbt being Experimental
> > before kernel 4.16? so the change in "minimum log size" is an on-disk format
> > change prior to removing the Experimental label??
>
> TBH nobody should be using reflink/rmap on 4.14 kernels, ever. :D

I agree, as these error messages try to express:
[    4.982926] XFS (vdd): EXPERIMENTAL reverse mapping btree feature
enabled. Use at your own risk!
[    4.984843] XFS (vdd): EXPERIMENTAL reflink feature enabled. Use at
your own risk!
[    4.987259] XFS (vdd): Log size 3693 blocks too small, minimum size
is 4473 blocks

But it is still a regression, because as I understand some where using
reflink back from
stable 4.9, while it was still maintained...

>
> That said... does it change the minimum log size for (finobt, !reflink,
> !rmap) filesystems?  That might be a bigger worry.  I /think/ the

No problem on my systems mounting small fs with (finobt, !reflink,!rmap)
formatted with mkfs.xfs 4.18.

> transaction reservation change is fine, though I defer to Amir on
> testing... :)
>

Testing passed as I wrote, for configs 4k,1k,reflink,reflink+overlay.

Sasha, please consider the fix patch for 4.14.y,4.9.y.

Thanks,
Amir.

Sasha Levin Nov. 7, 2018, 10:56 p.m. UTC | #5

On Wed, Nov 07, 2018 at 07:31:31AM +0200, Amir Goldstein wrote:
>On Wed, Nov 7, 2018 at 7:18 AM Darrick J. Wong <[e-mail]> wrote:
>>
>> On Wed, Nov 07, 2018 at 07:09:42AM +0200, Amir Goldstein wrote:
>> > On Mon, Nov 5, 2018 at 7:57 PM Amir Goldstein <[e-mail]> wrote:
>> > >
>> > > On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <[e-mail]> wrote:
>> > > >
>> > > > From: Brian Foster <[e-mail]>
>> > > >
>> > > > The truncate transaction does not ever modify the inode btree, but
>> > > > includes an associated log reservation. Update
>> > > > xfs_calc_itruncate_reservation() to remove the reservation
>> > > > associated with inobt updates.
>> > > >
>> > > > [Amir:  This commit was merged for kernel v4.16 and a twin commit was
>> > > >         merged for xfsprogs v4.16. As a result, a small xfs filesystem
>> > > >         formatted with features -m rmapbt=1,reflink=1 using mkfs.xfs
>> > > >         version >= v4.16 cannot be mounted with kernel < v4.16.
>> > > >
>> > > >         For example, xfstests generic/17{1,2,3} format a small fs and
>> > > >         when trying to mount it, they fail with an assert on this very
>> > > >         demonic line:
>> > > >
>> > > >  XFS (vdc): Log size 3075 blocks too small, minimum size is 3717 blocks
>> > > >  XFS (vdc): AAIEEE! Log failed size checks. Abort!
>> > > >  XFS: Assertion failed: 0, file: src/linux/fs/xfs/xfs_log.c, line: 666
>> > > >
>> > > >         The simple solution for stable kernels is to apply this patch,
>> > > >         because mkfs.xfs v4.16 is already in the wild, so we have to
>> > > >         assume that xfs filesystems with a "too small" log exist.
>> > > >         Regardless, xfsprogs maintainers should also consider reverting
>> > > >         the twin patch to stop creating those filesystems for the sake
>> > > >         of users with unpatched kernels.]
>> > > >
>> > > > Signed-off-by: Brian Foster <[e-mail]>
>> > > > Reviewed-by: Dave Chinner <[e-mail]>
>> > > > Reviewed-by: Darrick J. Wong <[e-mail]>
>> > > > Signed-off-by: Darrick J. Wong <[e-mail]>
>> > > > Cc: <[e-mail]> # v4.9+
>> > > > Signed-off-by: Amir Goldstein <[e-mail]>
>> > > > ---
>> > > >
>> > > > Darrick/Dave,
>> > > >
>> > > > It took me a while to figure out what was going on with my test systems
>> > > > when small test partitions (10G) stopped working with older kernels.
>> > > >
>> > > > Please bless this change for stable and consider the remedie for mkfs.xfs
>> > > > I verified that patch cleanly applies to stable kernels 4.14.y and 4.9.y
>> > > > and that I can mount a filsystem created with new mkfs.xfs.
>> > > >
>> > > > I am now running quick tests on stable 4.14.y with configs 4k, 1k,
>> > > > reflink,reflink+overlay to verify no regressions from this patch.
>> > > >
>> > >
>> > > FYI no regressions detected.
>> > >
>> > > Thoughts?
>> > >
>> >
>> > Maybe you'd want to chalk it up to reflink/rmapbt being Experimental
>> > before kernel 4.16? so the change in "minimum log size" is an on-disk format
>> > change prior to removing the Experimental label??
>>
>> TBH nobody should be using reflink/rmap on 4.14 kernels, ever. :D
>
>I agree, as these error messages try to express:
>[    4.982926] XFS (vdd): EXPERIMENTAL reverse mapping btree feature
>enabled. Use at your own risk!
>[    4.984843] XFS (vdd): EXPERIMENTAL reflink feature enabled. Use at
>your own risk!
>[    4.987259] XFS (vdd): Log size 3693 blocks too small, minimum size
>is 4473 blocks
>
>But it is still a regression, because as I understand some where using
>reflink back from
>stable 4.9, while it was still maintained...
>
>>
>> That said... does it change the minimum log size for (finobt, !reflink,
>> !rmap) filesystems?  That might be a bigger worry.  I /think/ the
>
>No problem on my systems mounting small fs with (finobt, !reflink,!rmap)
>formatted with mkfs.xfs 4.18.
>
>> transaction reservation change is fine, though I defer to Amir on
>> testing... :)
>>
>
>Testing passed as I wrote, for configs 4k,1k,reflink,reflink+overlay.
>
>Sasha, please consider the fix patch for 4.14.y,4.9.y.

Queued for 4.9 and 4.14, thank you.

--
Thanks,
Sasha

Patch

diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 6bd916bd35e2..48eff18c5496 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -232,8 +232,6 @@  xfs_calc_write_reservation(
  *    the super block to reflect the freed blocks: sector size
  *    worst case split in allocation btrees per extent assuming 4 extents:
  *		4 exts * 2 trees * (2 * max depth - 1) * block size
- *    the inode btree: max depth * blocksize
- *    the allocation btrees: 2 trees * (max depth - 1) * block size
  */
 STATIC uint
 xfs_calc_itruncate_reservation(
@@ -245,12 +243,7 @@  xfs_calc_itruncate_reservation(
 				      XFS_FSB_TO_B(mp, 1))),
 		    (xfs_calc_buf_res(9, mp->m_sb.sb_sectsize) +
 		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 4),
-				      XFS_FSB_TO_B(mp, 1)) +
-		    xfs_calc_buf_res(5, 0) +
-		    xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
-				     XFS_FSB_TO_B(mp, 1)) +
-		    xfs_calc_buf_res(2 + mp->m_ialloc_blks +
-				     mp->m_in_maxlevels, 0)));
+				      XFS_FSB_TO_B(mp, 1))));
 }
 
 /*