Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] xfs: xfs_trim_balloon() takes exclusive locks and walks balloon regardless of range - [Low] xfs: xfs_trim_balloon_run() minlen to sector conversion wraps in 32 bits --
commit 64c2bc84bef60128cd1c02bff575680cb7783bc1 Author: Denis V. Lunev <[email protected]> xfs: discard the balloon on FITRIM This commit makes xfs_ioc_trim() walk the balloon inode after the data device free space walk. It discards the balloon's unwritten extents that fall in the requested range, merging physically adjacent ones and issuing the discard in chunks. > diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c > --- a/fs/xfs/xfs_discard.c > +++ b/fs/xfs/xfs_discard.c [ ... ] > +static int > +xfs_trim_balloon_run( > + struct xfs_mount *mp, > + xfs_daddr_t daddr, > + uint64_t len, > + xfs_daddr_t start, > + xfs_daddr_t end, > + xfs_extlen_t minlen) > +{ [ ... ] > + if (last < daddr || last - daddr + 1 < XFS_FSB_TO_BB(mp, minlen)) > + return 0; [Severity: Low] Can the minlen conversion wrap here? In xfs_ioc_trim(), minlen is an xfs_extlen_t (u32) set from XFS_B_TO_FSB(mp, range.minlen). XFS_FSB_TO_BB() is defined as ((fsbno) << (mp)->m_blkbb_log) with no widening, so in xfs_trim_balloon_run() the shift is done in 32 bits before the comparison with the 64-bit run length. Once minlen << m_blkbb_log reaches 2^32, the result wraps and the minlen filter can be bypassed, so runs shorter than the requested minlen get discarded. Would casting minlen to xfs_daddr_t or u64 before the shift avoid this? [ ... ] > +static int > +xfs_trim_balloon( > + struct xfs_mount *mp, > + xfs_daddr_t start, > + xfs_daddr_t end, > + xfs_extlen_t minlen) > +{ [ ... ] > + /* > + * Nobody may write the balloon meanwhile: data written into an > + * unwritten extent would be discarded with it. > + */ > + xfs_ilock(ip, XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL); > + inode_dio_wait(VFS_I(ip)); > + error = filemap_write_and_wait(VFS_I(ip)->i_mapping); > + if (error) > + goto out_unlock; [Severity: Medium] Is it intended that every FITRIM takes these locks and scans the whole balloon, even when the requested range cannot overlap it? xfs_ioc_trim() calls xfs_trim_balloon() whenever the data device supports discard. If m_balloon_ino is set, xfs_trim_balloon() takes XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL, runs inode_dio_wait() and filemap_write_and_wait(), and walks the extent map from offset 0 to XFS_MAX_FILEOFF. None of this is bounded by the [start, end] range. The range is only applied per run in xfs_trim_balloon_run(). As a result, a FITRIM on a narrow range still blocks balloon inflate and shrink and does the full flush and scan. The commit message says "Every FITRIM discards the balloon again, which costs little: dm-ploop completes the discard of an unmapped cluster at once." Does that statement cover the exclusive locks, the DIO wait, the page cache flush and the full extent walk, which are taken regardless of the requested range? [ ... ] -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=3 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
