| Message ID | 1516696944-11996-1-git-send-email-shan.hai@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jan 23, 2018 at 04:42:24PM +0800, Shan Hai wrote: > Remove the extent size hint and realtime inode relevant code from > the xfs_bmapi_reserve_delalloc since it is not called on the inode > with extent size hint set or on a realtime inode. > > Signed-off-by: Shan Hai <shan.hai@oracle.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 32 ++++++++------------------------ > 1 file changed, 8 insertions(+), 24 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 1bddbba6b80c..8b4ab1932b58 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -3876,8 +3876,6 @@ xfs_bmapi_reserve_delalloc( > struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > xfs_extlen_t alen; > xfs_extlen_t indlen; > - char rt = XFS_IS_REALTIME_INODE(ip); > - xfs_extlen_t extsz; > int error; > xfs_fileoff_t aoff = off; > > @@ -3892,31 +3890,25 @@ xfs_bmapi_reserve_delalloc( > prealloc = alen - len; > > /* Figure out the extent size, adjust alen */ > - if (whichfork == XFS_COW_FORK) > - extsz = xfs_get_cowextsz_hint(ip); > - else > - extsz = xfs_get_extsz_hint(ip); This patch reminded me that I'd been wondering since the early days of reflink development -- why /does/ this function have the ability to align delalloc reservations, considering that none of the write paths seem to use it? Was this just some nugget from the old days? Will the CoW fork be ok if its tries to cowextsize align its da reservations? So I went digging into the git history and came up with aff3a9edb70 wherein Dave discovered that the result of these larger da reservations was that the whole extent could get allocated, but left some kind of stale data exposure problem. (I guess because we shoved the whole real extent into the extent map? I wasn't around in the 3.4 days...) Therefore, the easiest solution was to go straight to making an unwritten allocation, and any subsequent read would always return zeroes, the same as directio does. The CoW fork doesn't have this problem because even if we create large da reservations and then allocate the whole extent including parts that aren't mapped by the page cache, the CoW fork extents aren't used for reads and they aren't mapped into the (permanent) data fork unless we actually write something to that range. > - if (extsz) { > + if (whichfork == XFS_COW_FORK) { > struct xfs_bmbt_irec prev; > + xfs_extlen_t extsz = xfs_get_cowextsz_hint(ip); Please line up the variable names: struct xfs_bmbt_irec prev; xfs_extlen_t extsz = xfs_get_cowextsz_hint(ip); Otherwise it looks ok. I think I'll just edit this on its way in. Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > > if (!xfs_iext_peek_prev_extent(ifp, icur, &prev)) > prev.br_startoff = NULLFILEOFF; > > - error = xfs_bmap_extsize_align(mp, got, &prev, extsz, rt, eof, > + error = xfs_bmap_extsize_align(mp, got, &prev, extsz, 0, eof, > 1, 0, &aoff, &alen); > ASSERT(!error); > } > > - if (rt) > - extsz = alen / mp->m_sb.sb_rextsize; > - > /* > * Make a transaction-less quota reservation for delayed allocation > * blocks. This number gets adjusted later. We return if we haven't > * allocated blocks already inside this loop. > */ > error = xfs_trans_reserve_quota_nblks(NULL, ip, (long)alen, 0, > - rt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS); > + XFS_QMOPT_RES_REGBLKS); > if (error) > return error; > > @@ -3927,12 +3919,7 @@ xfs_bmapi_reserve_delalloc( > indlen = (xfs_extlen_t)xfs_bmap_worst_indlen(ip, alen); > ASSERT(indlen > 0); > > - if (rt) { > - error = xfs_mod_frextents(mp, -((int64_t)extsz)); > - } else { > - error = xfs_mod_fdblocks(mp, -((int64_t)alen), false); > - } > - > + error = xfs_mod_fdblocks(mp, -((int64_t)alen), false); > if (error) > goto out_unreserve_quota; > > @@ -3963,14 +3950,11 @@ xfs_bmapi_reserve_delalloc( > return 0; > > out_unreserve_blocks: > - if (rt) > - xfs_mod_frextents(mp, extsz); > - else > - xfs_mod_fdblocks(mp, alen, false); > + xfs_mod_fdblocks(mp, alen, false); > out_unreserve_quota: > if (XFS_IS_QUOTA_ON(mp)) > - xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0, rt ? > - XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS); > + xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0, > + XFS_QMOPT_RES_REGBLKS); > return error; > } > > -- > 2.14.1 > > -- > 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 -- 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
On 2018年01月24日 08:13, Darrick J. Wong wrote: > On Tue, Jan 23, 2018 at 04:42:24PM +0800, Shan Hai wrote: >> Remove the extent size hint and realtime inode relevant code from >> the xfs_bmapi_reserve_delalloc since it is not called on the inode >> with extent size hint set or on a realtime inode. >> >> Signed-off-by: Shan Hai <shan.hai@oracle.com> >> --- >> fs/xfs/libxfs/xfs_bmap.c | 32 ++++++++------------------------ >> 1 file changed, 8 insertions(+), 24 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c >> index 1bddbba6b80c..8b4ab1932b58 100644 >> --- a/fs/xfs/libxfs/xfs_bmap.c >> +++ b/fs/xfs/libxfs/xfs_bmap.c >> @@ -3876,8 +3876,6 @@ xfs_bmapi_reserve_delalloc( >> struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); >> xfs_extlen_t alen; >> xfs_extlen_t indlen; >> - char rt = XFS_IS_REALTIME_INODE(ip); >> - xfs_extlen_t extsz; >> int error; >> xfs_fileoff_t aoff = off; >> >> @@ -3892,31 +3890,25 @@ xfs_bmapi_reserve_delalloc( >> prealloc = alen - len; >> >> /* Figure out the extent size, adjust alen */ >> - if (whichfork == XFS_COW_FORK) >> - extsz = xfs_get_cowextsz_hint(ip); >> - else >> - extsz = xfs_get_extsz_hint(ip); > This patch reminded me that I'd been wondering since the early days of > reflink development -- why /does/ this function have the ability to > align delalloc reservations, considering that none of the write paths > seem to use it? Was this just some nugget from the old days? Will the > CoW fork be ok if its tries to cowextsize align its da reservations? > > So I went digging into the git history and came up with aff3a9edb70 > wherein Dave discovered that the result of these larger da reservations > was that the whole extent could get allocated, but left some kind of > stale data exposure problem. (I guess because we shoved the whole real > extent into the extent map? I wasn't around in the 3.4 days...) > Therefore, the easiest solution was to go straight to making an > unwritten allocation, and any subsequent read would always return > zeroes, the same as directio does. > > The CoW fork doesn't have this problem because even if we create large > da reservations and then allocate the whole extent including parts that > aren't mapped by the page cache, the CoW fork extents aren't used for > reads and they aren't mapped into the (permanent) data fork unless we > actually write something to that range. > >> - if (extsz) { >> + if (whichfork == XFS_COW_FORK) { >> struct xfs_bmbt_irec prev; >> + xfs_extlen_t extsz = xfs_get_cowextsz_hint(ip); Hi Darrick, > Please line up the variable names: > > struct xfs_bmbt_irec prev; > xfs_extlen_t extsz = xfs_get_cowextsz_hint(ip); > > Otherwise it looks ok. I think I'll just edit this on its way in. Sorry about that, I will be more careful in the future. Thanks for the review. Regards Shan Hai > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > --D > >> >> if (!xfs_iext_peek_prev_extent(ifp, icur, &prev)) >> prev.br_startoff = NULLFILEOFF; >> >> - error = xfs_bmap_extsize_align(mp, got, &prev, extsz, rt, eof, >> + error = xfs_bmap_extsize_align(mp, got, &prev, extsz, 0, eof, >> 1, 0, &aoff, &alen); >> ASSERT(!error); >> } >> >> - if (rt) >> - extsz = alen / mp->m_sb.sb_rextsize; >> - >> /* >> * Make a transaction-less quota reservation for delayed allocation >> * blocks. This number gets adjusted later. We return if we haven't >> * allocated blocks already inside this loop. >> */ >> error = xfs_trans_reserve_quota_nblks(NULL, ip, (long)alen, 0, >> - rt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS); >> + XFS_QMOPT_RES_REGBLKS); >> if (error) >> return error; >> >> @@ -3927,12 +3919,7 @@ xfs_bmapi_reserve_delalloc( >> indlen = (xfs_extlen_t)xfs_bmap_worst_indlen(ip, alen); >> ASSERT(indlen > 0); >> >> - if (rt) { >> - error = xfs_mod_frextents(mp, -((int64_t)extsz)); >> - } else { >> - error = xfs_mod_fdblocks(mp, -((int64_t)alen), false); >> - } >> - >> + error = xfs_mod_fdblocks(mp, -((int64_t)alen), false); >> if (error) >> goto out_unreserve_quota; >> >> @@ -3963,14 +3950,11 @@ xfs_bmapi_reserve_delalloc( >> return 0; >> >> out_unreserve_blocks: >> - if (rt) >> - xfs_mod_frextents(mp, extsz); >> - else >> - xfs_mod_fdblocks(mp, alen, false); >> + xfs_mod_fdblocks(mp, alen, false); >> out_unreserve_quota: >> if (XFS_IS_QUOTA_ON(mp)) >> - xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0, rt ? >> - XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS); >> + xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0, >> + XFS_QMOPT_RES_REGBLKS); >> return error; >> } >> >> -- >> 2.14.1 >> >> -- >> 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 -- 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
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 1bddbba6b80c..8b4ab1932b58 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -3876,8 +3876,6 @@ xfs_bmapi_reserve_delalloc( struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); xfs_extlen_t alen; xfs_extlen_t indlen; - char rt = XFS_IS_REALTIME_INODE(ip); - xfs_extlen_t extsz; int error; xfs_fileoff_t aoff = off; @@ -3892,31 +3890,25 @@ xfs_bmapi_reserve_delalloc( prealloc = alen - len; /* Figure out the extent size, adjust alen */ - if (whichfork == XFS_COW_FORK) - extsz = xfs_get_cowextsz_hint(ip); - else - extsz = xfs_get_extsz_hint(ip); - if (extsz) { + if (whichfork == XFS_COW_FORK) { struct xfs_bmbt_irec prev; + xfs_extlen_t extsz = xfs_get_cowextsz_hint(ip); if (!xfs_iext_peek_prev_extent(ifp, icur, &prev)) prev.br_startoff = NULLFILEOFF; - error = xfs_bmap_extsize_align(mp, got, &prev, extsz, rt, eof, + error = xfs_bmap_extsize_align(mp, got, &prev, extsz, 0, eof, 1, 0, &aoff, &alen); ASSERT(!error); } - if (rt) - extsz = alen / mp->m_sb.sb_rextsize; - /* * Make a transaction-less quota reservation for delayed allocation * blocks. This number gets adjusted later. We return if we haven't * allocated blocks already inside this loop. */ error = xfs_trans_reserve_quota_nblks(NULL, ip, (long)alen, 0, - rt ? XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS); + XFS_QMOPT_RES_REGBLKS); if (error) return error; @@ -3927,12 +3919,7 @@ xfs_bmapi_reserve_delalloc( indlen = (xfs_extlen_t)xfs_bmap_worst_indlen(ip, alen); ASSERT(indlen > 0); - if (rt) { - error = xfs_mod_frextents(mp, -((int64_t)extsz)); - } else { - error = xfs_mod_fdblocks(mp, -((int64_t)alen), false); - } - + error = xfs_mod_fdblocks(mp, -((int64_t)alen), false); if (error) goto out_unreserve_quota; @@ -3963,14 +3950,11 @@ xfs_bmapi_reserve_delalloc( return 0; out_unreserve_blocks: - if (rt) - xfs_mod_frextents(mp, extsz); - else - xfs_mod_fdblocks(mp, alen, false); + xfs_mod_fdblocks(mp, alen, false); out_unreserve_quota: if (XFS_IS_QUOTA_ON(mp)) - xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0, rt ? - XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS); + xfs_trans_unreserve_quota_nblks(NULL, ip, (long)alen, 0, + XFS_QMOPT_RES_REGBLKS); return error; }
Remove the extent size hint and realtime inode relevant code from the xfs_bmapi_reserve_delalloc since it is not called on the inode with extent size hint set or on a realtime inode. Signed-off-by: Shan Hai <shan.hai@oracle.com> --- fs/xfs/libxfs/xfs_bmap.c | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-)