Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Medium] ext4: FITRIM balloon discard races with writeback and shared-lock DIO on unwritten extents - [Medium] ext4: ext4_trim_balloon() bounds its walk by i_size and misses unwritten extents past EOF - [Low] ext4: 64-bit DIV_ROUND_UP() on i_size in ext4_trim_balloon() breaks 32-bit builds - [Low] ext4: FITRIM re-discards and re-counts the whole balloon on every call --
commit d12d49f2b58d226c5ae16e1ae2d56d927c53b382 Author: Denis V. Lunev <[email protected]> ext4: discard the balloon on FITRIM This commit makes FITRIM walk the balloon inode after the free space and discard its unwritten extents inside the requested range. Physically adjacent extents are merged into one discard. The commit message says the shared i_rwsem keeps the balloon blocks stable during the discard. Link: https://virtuozzo.atlassian.net/browse/VSTOR-153590 > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c [ ... ] > +static int ext4_trim_balloon_run(struct super_block *sb, ext4_fsblk_t pblk, > + ext4_fsblk_t len, ext4_fsblk_t start, > + ext4_fsblk_t end, ext4_fsblk_t minblocks, > + u64 *trimmed) > +{ > [ ... ] > + len = last - pblk + 1; > + trace_ext4_discard_blocks(sb, pblk, len); > + *trimmed += len; > + return sb_issue_discard(sb, pblk, len, GFP_NOFS, 0); > +} [Severity: Medium] The commit message says "The shared i_rwsem keeps fallocate, truncate and EXT4_IOC_MOVE_EXT away from the balloon meanwhile" and "An unwritten extent reads as zeroes whatever the device returns, so the discard changes nothing visible". Can this discard destroy data that is being written into the balloon? The balloon file is handed to root by ext4_open_balloon() in fs/ext4/ioctl.c as an O_RDWR file (when mnt_want_write() succeeds) using ext4_file_operations, so nothing enforces that it never holds data. Two paths do not appear to be excluded by the shared i_rwsem taken in ext4_trim_balloon(): buffered write into a fallocated unwritten extent i_rwsem exclusive, dirty pages left, lock dropped writeback does not take i_rwsem; conversion happens at I/O end ext4_trim_balloon() ext4_map_blocks() still reports EXT4_MAP_UNWRITTEN sb_issue_discard() <- no filemap_write_and_wait_range() ext4_dio_write_iter() inode_lock_shared() for a non-extending overwrite (see ext4_dio_write_checks()) ext4_trim_balloon() shared lock taken concurrently, discards blocks being written In both cases the discard may be issued after or during the data write, and the extent is then converted to written. Should ext4_trim_balloon() flush the range first, take i_rwsem exclusive, or should the open path prevent data writes? As written, the commit message's safety argument seems incomplete for these cases. > +static int ext4_trim_balloon(struct super_block *sb, ext4_fsblk_t start, > + ext4_fsblk_t end, ext4_fsblk_t minblocks, > + u64 *trimmed) > +{ > [ ... ] > + inode_lock_shared(inode); > + nr_lblks = min_t(u64, EXT_MAX_BLOCKS, > + DIV_ROUND_UP(i_size_read(inode), sb->s_blocksize)); [Severity: Low] Does this DIV_ROUND_UP() build on 32-bit targets? i_size_read() returns a 64-bit loff_t and sb->s_blocksize is unsigned long, so the plain '/' in DIV_ROUND_UP() should become a libgcc 64-bit division (__udivdi3 or __aeabi_uldivmod), which the kernel does not link. Would a shift by sb->s_blocksize_bits (or inode->i_blkbits) with rounding avoid it? [Severity: Medium] Is bounding the walk by i_size correct for the stated goal? If the balloon has unwritten extents past EOF, for example after fallocate(FALLOC_FL_KEEP_SIZE) on the fd from EXT4_IOC_OPEN_BALLOON, or after i_size was lowered without freeing them, then nr_lblks does not cover them: nr_lblks = DIV_ROUND_UP(i_size, blocksize) for (lblk = 0; lblk < nr_lblks; ...) <- never maps blocks past EOF Those blocks are allocated, so ext4_trim_all_free() skips them as well, and they are never discarded. I could not confirm which fallocate flags the userspace balloon tool uses, so this may depend on that. Since ext4_map_blocks() returns holes in a single call, would walking up to EXT_MAX_BLOCKS be cheap enough? > + if (!ret && !ext4_trim_interrupted()) > + ret = ext4_trim_balloon_run(sb, run_pblk, run_len, start, end, > + minblocks, trimmed); [ ... ] > @@ -7103,6 +7193,13 @@ int ext4_trim_fs(struct super_block *sb, struct > fstrim_range *range) > first_cluster = 0; > } > > + if (!ret && !ext4_trim_interrupted()) { > + ret = ext4_trim_balloon(sb, start, last_blk, > + EXT4_C2B(EXT4_SB(sb), minlen), > + &balloon_blks); > + trimmed += EXT4_NUM_B2C(EXT4_SB(sb), balloon_blks); > + } [Severity: Low] Is it intended that the whole balloon is re-discarded and re-counted on every FITRIM? ext4_trim_all_free() skips groups already trimmed via EXT4_MB_GRP_WAS_TRIMMED and s_last_trim_minblks, but the balloon walk has no equivalent gate: FITRIM -> ext4_trim_fs() -> ext4_trim_balloon() -> ext4_trim_balloon_run() sb_issue_discard() for every unwritten run in range Each call also adds the full balloon size to trimmed, so range->len (and the fstrim -v output) reports the balloon as newly trimmed every time. The commit message says "repeating it is harmless", but this covers only the data, not the repeated I/O or the reported length. Also, start and last_blk are block-granular, while the free-space path converts them to cluster offsets through ext4_get_group_no_and_offset(). On bigalloc, can a FITRIM range that begins or ends mid-cluster produce a discard that only partly covers a cluster, which the commit message says it is trying to avoid? -- Sashiko AI review ยท https://sashiko.vzint.dev/#/patchset/[email protected]?part=1 _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
