Message ID | 20181103171524.2740-1-amir73il@gmail.com (mailing list archive) |
---|---|
State | Accepted, archived |
Headers | show |
Series | xfs: truncate transaction does not modify the inobt | expand |
On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <amir73il@gmail.com> wrote: > > From: Brian Foster <bfoster@redhat.com> > > 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 <bfoster@redhat.com> > Reviewed-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > Cc: <stable@vger.kernel.org> # v4.9+ > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > 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 >
On Mon, Nov 5, 2018 at 7:57 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > From: Brian Foster <bfoster@redhat.com> > > > > 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 <bfoster@redhat.com> > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > Cc: <stable@vger.kernel.org> # v4.9+ > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > 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.
On Wed, Nov 07, 2018 at 07:09:42AM +0200, Amir Goldstein wrote: > On Mon, Nov 5, 2018 at 7:57 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > From: Brian Foster <bfoster@redhat.com> > > > > > > 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 <bfoster@redhat.com> > > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > Cc: <stable@vger.kernel.org> # v4.9+ > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > > > > 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.
On Wed, Nov 7, 2018 at 7:18 AM Darrick J. Wong <darrick.wong@oracle.com> 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 <amir73il@gmail.com> wrote: > > > > > > On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > > > From: Brian Foster <bfoster@redhat.com> > > > > > > > > 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 <bfoster@redhat.com> > > > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > Cc: <stable@vger.kernel.org> # v4.9+ > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > --- > > > > > > > > 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.
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 <darrick.wong@oracle.com> 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 <amir73il@gmail.com> wrote: >> > > >> > > On Sat, Nov 3, 2018 at 7:15 PM Amir Goldstein <amir73il@gmail.com> wrote: >> > > > >> > > > From: Brian Foster <bfoster@redhat.com> >> > > > >> > > > 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 <bfoster@redhat.com> >> > > > Reviewed-by: Dave Chinner <dchinner@redhat.com> >> > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> >> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> >> > > > Cc: <stable@vger.kernel.org> # v4.9+ >> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> >> > > > --- >> > > > >> > > > 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
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)))); } /*