| Message ID | 158993916213.976105.11958914131452778480.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: refactor incore inode walking | expand |
On Tue, May 19, 2020 at 06:46:02PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Refactor the two eofb-matching logics into a single helper so that we > don't repeat ourselves. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_icache.c | 59 +++++++++++++++++++++++++++------------------------ > 1 file changed, 31 insertions(+), 28 deletions(-) > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > index ac66e7d8698d..1f12c6a0c48e 100644 > --- a/fs/xfs/xfs_icache.c > +++ b/fs/xfs/xfs_icache.c > @@ -1436,6 +1436,33 @@ xfs_inode_match_id_union( > return 0; > } > > +/* > + * Is this inode @ip eligible for eof/cow block reclamation, given some > + * filtering parameters @eofb? The inode is eligible if @eofb is null or > + * if the predicate functions match. > + */ > +static bool > +xfs_inode_matches_eofb( > + struct xfs_inode *ip, > + struct xfs_eofblocks *eofb) > +{ > + int match; > + > + if (!eofb) > + return true; > + > + if (eofb->eof_flags & XFS_EOF_FLAGS_UNION) > + match = xfs_inode_match_id_union(ip, eofb); > + else > + match = xfs_inode_match_id(ip, eofb); > + if (match) > + return true; > + > + /* skip the inode if the file size is too small */ > + return !(eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE && > + XFS_ISIZE(ip) < eofb->eof_min_file_size); This looks wrong - the size check should be applied if we did already find a match and not override it based on the current code, e.g.: if (eofb->eof_flags & XFS_EOF_FLAGS_UNION) match = xfs_inode_match_id_union(ip, eofb); else match = xfs_inode_match_id(ip, eofb); if (match) { /* skip the inode if the file size is too small */ if ((eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE) && XFS_ISIZE(ip) < eofb->eof_min_file_size) return false; } return match;
On Wed, May 20, 2020 at 08:42:11AM +0200, Christoph Hellwig wrote: > On Tue, May 19, 2020 at 06:46:02PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Refactor the two eofb-matching logics into a single helper so that we > > don't repeat ourselves. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/xfs_icache.c | 59 +++++++++++++++++++++++++++------------------------ > > 1 file changed, 31 insertions(+), 28 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c > > index ac66e7d8698d..1f12c6a0c48e 100644 > > --- a/fs/xfs/xfs_icache.c > > +++ b/fs/xfs/xfs_icache.c > > @@ -1436,6 +1436,33 @@ xfs_inode_match_id_union( > > return 0; > > } > > > > +/* > > + * Is this inode @ip eligible for eof/cow block reclamation, given some > > + * filtering parameters @eofb? The inode is eligible if @eofb is null or > > + * if the predicate functions match. > > + */ > > +static bool > > +xfs_inode_matches_eofb( > > + struct xfs_inode *ip, > > + struct xfs_eofblocks *eofb) > > +{ > > + int match; > > + > > + if (!eofb) > > + return true; > > + > > + if (eofb->eof_flags & XFS_EOF_FLAGS_UNION) > > + match = xfs_inode_match_id_union(ip, eofb); > > + else > > + match = xfs_inode_match_id(ip, eofb); > > + if (match) > > + return true; > > + > > + /* skip the inode if the file size is too small */ > > + return !(eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE && > > + XFS_ISIZE(ip) < eofb->eof_min_file_size); > > This looks wrong - the size check should be applied if we did already > find a match and not override it based on the current code, e.g.: > > if (eofb->eof_flags & XFS_EOF_FLAGS_UNION) > match = xfs_inode_match_id_union(ip, eofb); > else > match = xfs_inode_match_id(ip, eofb); > > if (match) { > /* skip the inode if the file size is too small */ > if ((eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE) && > XFS_ISIZE(ip) < eofb->eof_min_file_size) > return false; > } > > return match; Ah, I see what I did wrong here; the size check was another opportunity for us to say no even if the inode matched. Ok, I'll go fix it. --D
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index ac66e7d8698d..1f12c6a0c48e 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -1436,6 +1436,33 @@ xfs_inode_match_id_union( return 0; } +/* + * Is this inode @ip eligible for eof/cow block reclamation, given some + * filtering parameters @eofb? The inode is eligible if @eofb is null or + * if the predicate functions match. + */ +static bool +xfs_inode_matches_eofb( + struct xfs_inode *ip, + struct xfs_eofblocks *eofb) +{ + int match; + + if (!eofb) + return true; + + if (eofb->eof_flags & XFS_EOF_FLAGS_UNION) + match = xfs_inode_match_id_union(ip, eofb); + else + match = xfs_inode_match_id(ip, eofb); + if (match) + return true; + + /* skip the inode if the file size is too small */ + return !(eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE && + XFS_ISIZE(ip) < eofb->eof_min_file_size); +} + STATIC int xfs_inode_free_eofblocks( struct xfs_inode *ip, @@ -1443,7 +1470,6 @@ xfs_inode_free_eofblocks( { struct xfs_eofblocks *eofb = args; bool wait; - int match; int ret = 0; wait = (eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC)); @@ -1462,19 +1488,8 @@ xfs_inode_free_eofblocks( if (!wait && mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY)) return 0; - if (eofb) { - if (eofb->eof_flags & XFS_EOF_FLAGS_UNION) - match = xfs_inode_match_id_union(ip, eofb); - else - match = xfs_inode_match_id(ip, eofb); - if (!match) - return 0; - - /* skip the inode if the file size is too small */ - if (eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE && - XFS_ISIZE(ip) < eofb->eof_min_file_size) - return 0; - } + if (!xfs_inode_matches_eofb(ip, eofb)) + return 0; /* * If the caller is waiting, return -EAGAIN to keep the background @@ -1716,25 +1731,13 @@ xfs_inode_free_cowblocks( void *args) { struct xfs_eofblocks *eofb = args; - int match; int ret = 0; if (!xfs_prep_free_cowblocks(ip)) return 0; - if (eofb) { - if (eofb->eof_flags & XFS_EOF_FLAGS_UNION) - match = xfs_inode_match_id_union(ip, eofb); - else - match = xfs_inode_match_id(ip, eofb); - if (!match) - return 0; - - /* skip the inode if the file size is too small */ - if (eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE && - XFS_ISIZE(ip) < eofb->eof_min_file_size) - return 0; - } + if (!xfs_inode_matches_eofb(ip, eofb)) + return 0; /* Free the CoW blocks */ xfs_ilock(ip, XFS_IOLOCK_EXCL);