| Message ID | 20240429174746.2132161-10-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | block atomic writes for XFS | expand |
On Mon, Apr 29, 2024 at 05:47:34PM +0000, John Garry wrote: > For when forcealign is enabled, we want the EOF to be aligned as well, so > do not free EOF blocks. This is doesn't match what the code does. The code is correct - it rounds the range to be trimmed up to the aligned offset beyond EOF and then frees them. The description needs to be updated to reflect this. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > fs/xfs/xfs_bmap_util.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > index 19e11d1da660..f26d1570b9bd 100644 > --- a/fs/xfs/xfs_bmap_util.c > +++ b/fs/xfs/xfs_bmap_util.c > @@ -542,8 +542,13 @@ xfs_can_free_eofblocks( > * forever. > */ > end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip)); > - if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) > + > + /* Do not free blocks when forcing extent sizes */ > + if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) I see this sort of check all through the remaining patches. Given there are significant restrictions on forced alignment, shouldn't this all the details be pushed inside the helper function? e.g. /* * Forced extent alignment is dependent on extent size hints being * set to define the alignment. Alignment is only necessary when the * extent size hint is larger than a single block. * * If reflink is enabled on the file or we are in always_cow mode, * we can't easily do forced alignment. * * We don't support forced alignment on realtime files. * XXX(dgc): why not? */ static inline bool xfs_inode_has_forcealign(struct xfs_inode *ip) { if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE)) return false; if (ip->i_extsize <= 1) return false; if (xfs_is_cow_inode(ip)) return false; if (ip->di_flags & XFS_DIFLAG_REALTIME) return false; return ip->di_flags2 & XFS_DIFLAG2_FORCEALIGN; } -Dave.
On 30/04/2024 23:54, Dave Chinner wrote: > On Mon, Apr 29, 2024 at 05:47:34PM +0000, John Garry wrote: >> For when forcealign is enabled, we want the EOF to be aligned as well, so >> do not free EOF blocks. > > This is doesn't match what the code does. The code is correct - it > rounds the range to be trimmed up to the aligned offset beyond EOF > and then frees them. The description needs to be updated to reflect > this. ok, fine > >> >> Signed-off-by: John Garry <john.g.garry@oracle.com> >> --- >> fs/xfs/xfs_bmap_util.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c >> index 19e11d1da660..f26d1570b9bd 100644 >> --- a/fs/xfs/xfs_bmap_util.c >> +++ b/fs/xfs/xfs_bmap_util.c >> @@ -542,8 +542,13 @@ xfs_can_free_eofblocks( >> * forever. >> */ >> end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip)); >> - if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) >> + >> + /* Do not free blocks when forcing extent sizes */ >> + if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) > > I see this sort of check all through the remaining patches. > > Given there are significant restrictions on forced alignment, > shouldn't this all the details be pushed inside the helper function? > e.g. > > /* > * Forced extent alignment is dependent on extent size hints being > * set to define the alignment. Alignment is only necessary when the > * extent size hint is larger than a single block. > * > * If reflink is enabled on the file or we are in always_cow mode, > * we can't easily do forced alignment. > * > * We don't support forced alignment on realtime files. > * XXX(dgc): why not? There is no technical reason to not be able to support forcealign on RT, AFAIK. My idea is to support RT after non-RT is supported. > */ > static inline bool > xfs_inode_has_forcealign(struct xfs_inode *ip) > { > if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE)) > return false; > if (ip->i_extsize <= 1) > return false; > > if (xfs_is_cow_inode(ip)) > return false; Could we just include this in the forcealign validate checks? Currently we just check CoW extsize is zero there. > if (ip->di_flags & XFS_DIFLAG_REALTIME) > return false; We check this in xfs_inode_validate_forcealign() > > return ip->di_flags2 & XFS_DIFLAG2_FORCEALIGN; > } > So can we simply have: static inline bool xfs_inode_has_forcealign(struct xfs_inode *ip) { if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE)) return false; if (ip->i_extsize <= 1) return false; return ip->di_flags2 & XFS_DIFLAG2_FORCEALIGN; } Thanks, John
On Wed, May 01, 2024 at 09:30:37AM +0100, John Garry wrote: > On 30/04/2024 23:54, Dave Chinner wrote: > > On Mon, Apr 29, 2024 at 05:47:34PM +0000, John Garry wrote: > > > For when forcealign is enabled, we want the EOF to be aligned as well, so > > > do not free EOF blocks. > > > > This is doesn't match what the code does. The code is correct - it > > rounds the range to be trimmed up to the aligned offset beyond EOF > > and then frees them. The description needs to be updated to reflect > > this. > > ok, fine > > > > > > > > > Signed-off-by: John Garry <john.g.garry@oracle.com> > > > --- > > > fs/xfs/xfs_bmap_util.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c > > > index 19e11d1da660..f26d1570b9bd 100644 > > > --- a/fs/xfs/xfs_bmap_util.c > > > +++ b/fs/xfs/xfs_bmap_util.c > > > @@ -542,8 +542,13 @@ xfs_can_free_eofblocks( > > > * forever. > > > */ > > > end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip)); > > > - if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) > > > + > > > + /* Do not free blocks when forcing extent sizes */ > > > + if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) > > > > I see this sort of check all through the remaining patches. > > > > Given there are significant restrictions on forced alignment, > > shouldn't this all the details be pushed inside the helper function? > > e.g. > > > > /* > > * Forced extent alignment is dependent on extent size hints being > > * set to define the alignment. Alignment is only necessary when the > > * extent size hint is larger than a single block. > > * > > * If reflink is enabled on the file or we are in always_cow mode, > > * we can't easily do forced alignment. > > * > > * We don't support forced alignment on realtime files. > > * XXX(dgc): why not? > > There is no technical reason to not be able to support forcealign on RT, > AFAIK. My idea is to support RT after non-RT is supported. > > > */ > > static inline bool > > xfs_inode_has_forcealign(struct xfs_inode *ip) > > { > > if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE)) > > return false; > > if (ip->i_extsize <= 1) > > return false; > > > > if (xfs_is_cow_inode(ip)) > > return false; > > Could we just include this in the forcealign validate checks? Currently we > just check CoW extsize is zero there. Checking COW extsize is zero doesn't tell us anything useful about whether the inode might have shared extents, or that the filesystem has had the sysfs "always cow" debug knob turned on. That changes filesystem behaviour at mount time and has nothing to do with the on-disk format constraints. And now that I think about it, checking for COW extsize is completely the wrong thing to do because it doesn't get used until an extent is shared and a COW trigger is hit. So the presence of COW extsize has zero impact on whether we can use forced alignment or not. IOWs, we have to check for shared extents or always cow here, because even a file with correctly set up forced alignment needs to have forced alignment disabled when always_cow is enabled. Every write is going to use the COW path and AFAICT we don't support forced alignment through that path yet. > > > if (ip->di_flags & XFS_DIFLAG_REALTIME) > > return false; > > We check this in xfs_inode_validate_forcealign() That's kinda my point - we have a random smattering of different checks at different layers and in different contexts. i.e. There's no one function that performs -all- the "can we do forced alignment" checks that allow forced alignment to be used. This simply adds all those checks in the one place and ensures that even if other code gets checks wrong, we won't use forcealign inappropriately. -Dave.
On 02/05/2024 02:11, Dave Chinner wrote: >>> static inline bool >>> xfs_inode_has_forcealign(struct xfs_inode *ip) >>> { >>> if (!(ip->di_flags & XFS_DIFLAG_EXTSIZE)) >>> return false; >>> if (ip->i_extsize <= 1) >>> return false; >>> >>> if (xfs_is_cow_inode(ip)) >>> return false; >> Could we just include this in the forcealign validate checks? Currently we >> just check CoW extsize is zero there. > Checking COW extsize is zero doesn't tell us anything useful about > whether the inode might have shared extents, or that the filesystem > has had the sysfs "always cow" debug knob turned on. That changes > filesystem behaviour at mount time and has nothing to do with the > on-disk format constraints. > > And now that I think about it, checking for COW extsize is > completely the wrong thing to do because it doesn't get used until > an extent is shared and a COW trigger is hit. So the presence of COW > extsize has zero impact on whether we can use forced alignment or > not. ok > > IOWs, we have to check for shared extents or always cow here, > because even a file with correctly set up forced alignment needs to > have forced alignment disabled when always_cow is enabled. Every > write is going to use the COW path and AFAICT we don't support > forced alignment through that path yet. ok > >>> if (ip->di_flags & XFS_DIFLAG_REALTIME) >>> return false; >> We check this in xfs_inode_validate_forcealign() > That's kinda my point - we have a random smattering of different > checks at different layers and in different contexts. i.e. There's > no one function that performs -all- the "can we do forced alignment" > checks that allow forced alignment to be used. This simply adds all > those checks in the one place and ensures that even if other code > gets checks wrong, we won't use forcealign inappropriately. Fine, I can do that if you think it is the best strategy. Thanks, John
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index 19e11d1da660..f26d1570b9bd 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -542,8 +542,13 @@ xfs_can_free_eofblocks( * forever. */ end_fsb = XFS_B_TO_FSB(mp, (xfs_ufsize_t)XFS_ISIZE(ip)); - if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) + + /* Do not free blocks when forcing extent sizes */ + if (xfs_inode_has_forcealign(ip) && ip->i_extsize > 1) + end_fsb = roundup_64(end_fsb, ip->i_extsize); + else if (XFS_IS_REALTIME_INODE(ip) && mp->m_sb.sb_rextsize > 1) end_fsb = xfs_rtb_roundup_rtx(mp, end_fsb); + last_fsb = XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes); if (last_fsb <= end_fsb) return false;
For when forcealign is enabled, we want the EOF to be aligned as well, so do not free EOF blocks. Signed-off-by: John Garry <john.g.garry@oracle.com> --- fs/xfs/xfs_bmap_util.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)