Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] xfs: xfs_balloon_get() leaks inode ref on error, now reached by every FITRIM - [Medium] xfs: xfs_trim_balloon() discards extents sampled unwritten after dropping ILOCK - [Low] xfs: xfs_trim_balloon() walks only [0, i_size), skipping unwritten extents past EOF --
commit 53d905c890dd198401bff5f922e47e4a50094d23 Author: Denis V. Lunev <[email protected]> xfs: discard the balloon on FITRIM This commit makes xfs_ioc_trim() walk the balloon inode after the free space of the data device. It discards the unwritten extents that fall in the requested range, merging physically adjacent ones into one discard. Link: https://virtuozzo.atlassian.net/browse/VSTOR-153590 > 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, > + xfs_daddr_t len, > + xfs_daddr_t start, > + xfs_daddr_t end, > + xfs_extlen_t minlen) > +{ [ ... ] > + return blkdev_issue_discard(mp->m_ddev_targp->bt_bdev, daddr, > + last - daddr + 1, GFP_NOFS); > +} [ ... ] > + ip = xfs_balloon_get(mp, balloon_ino, 0); > + if (IS_ERR(ip)) > + return 0; [Severity: Medium] Can this leak an inode reference on every FITRIM? xfs_balloon_get() calls xfs_iget() and then returns an error without releasing the inode when the mode check fails: fs/xfs/xfs_super.c:xfs_balloon_get() { ... error = xfs_iget(mp, NULL, balloon_ino, flags, 0, &ip); if (error) return ERR_PTR(error); inode = VFS_I(ip); if (!S_ISREG(inode->i_mode) || IS_IMMUTABLE(inode)) return ERR_PTR(-EINVAL); ... } This problem wasn't introduced by this patch in xfs_balloon_get() itself, but xfs_trim_balloon() is a new caller that reaches this error path on every FITRIM. It treats IS_ERR(ip) as nothing to do and returns 0 with no pointer to release. If the balloon inode becomes immutable after mount (for example with chattr +i), each FITRIM would pin one more reference, which could trigger a busy-inode warning or block a clean unmount. Should xfs_balloon_get() call xfs_irele(ip) before returning -EINVAL? > + if (XFS_IS_REALTIME_INODE(ip)) > + goto out_rele; > + > + xfs_ilock(ip, XFS_IOLOCK_SHARED); > + end_fsb = XFS_B_TO_FSB(mp, i_size_read(VFS_I(ip))); [Severity: Low] Does this walk miss unwritten extents beyond EOF? The loop only covers [0, i_size): end_fsb = XFS_B_TO_FSB(mp, i_size_read(VFS_I(ip))); while (off < end_fsb) { The balloon file is opened through xfs_open_balloon() with the plain xfs_file_operations. fallocate(FALLOC_FL_KEEP_SIZE) is allowed by xfs_falloc_newsize() without extending i_size: fs/xfs/xfs_file.c:xfs_falloc_newsize() { if ((mode & FALLOC_FL_KEEP_SIZE) || offset + len <= i_size_read(inode)) return 0; ... } Unwritten extents allocated past EOF this way would never be examined, and the free space walk does not cover them either. The commit message says "A balloon inflated before this change is released by the next FITRIM." Is that accurate for a balloon with blocks past i_size? Would walking up to the last mapped extent, or to XFS_MAX_FILEOFF, be more appropriate than i_size? > + while (off < end_fsb) { > + if (xfs_trim_should_stop()) > + break; > + cond_resched(); > + > + nmaps = 1; > + lock_mode = xfs_ilock_data_map_shared(ip); > + error = xfs_bmapi_read(ip, off, end_fsb - off, &map, &nmaps, 0); > + xfs_iunlock(ip, lock_mode); [Severity: Medium] Can the discard race with a direct I/O write to the balloon? The extent state is sampled here under ILOCK_SHARED, which is dropped immediately. The discard is issued later from xfs_trim_balloon_run() at the next non-adjacent extent or after the loop, with no ILOCK held and no recheck that the extent is still unwritten. Only XFS_IOLOCK_SHARED is held across this, and aligned direct I/O overwrites also take only the shared IOLOCK: fs/xfs/xfs_file.c:xfs_file_dio_write_aligned() { unsigned int iolock = XFS_IOLOCK_SHARED; ... ret = iomap_dio_rw(iocb, from, ops, dops, 0, ac, 0); } xfs_open_balloon() hands out the balloon as O_RDWR when mnt_want_write() succeeds: fs/xfs/xfs_ioctl.c:xfs_open_balloon() { ro = mnt_want_write(path.mnt); if (ro) mode = O_RDONLY; else mode = O_RDWR; filp = alloc_file(&path, mode, &xfs_file_operations); } If a privileged writer issues an aligned O_DIRECT write to an unwritten balloon extent while xfs_trim_balloon() runs, could its data be discarded after the extent is converted to written? That would conflict with the commit message statement "An unwritten extent reads as zeroes whatever the device returns, so the discard changes nothing visible". The commit message also says "The shared IOLOCK keeps fallocate and truncate away from the balloon meanwhile". That holds, since those take the exclusive IOLOCK, but is it sufficient given that shared-IOLOCK writers are not excluded? > + if (error || !nmaps || !map.br_blockcount) > + break; [ ... ] -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=2 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
