Re: [PATCH v2] f2fs: reserve space to be shrunk during resize_fs

[Chao Yu <chao@kernel.org>]

Daeho, sorry for the delay.

On 10/23/26 00:55, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> f2fs_resize_fs() drops gc_lock and the cp lock between the free space
> check and freeze_super(). In that window, others can allocate the free
> space, so the second check after freeze_super() fails with -ENOSPC.
> This is easy to hit on a busy filesystem, e.g. at OTA install time.
> 
> Reserve the space to be shrunk via resize_reserved_blocks once the
> first check passes, and deduct it in __get_available_block_count() like
> alias_reserved_blocks. Release it on success or on error.
> 
> With the reservation, the second check should not fail anymore, so:
> - allow root reserved blocks, which root may use in the meantime,
> - recheck DISABLE_CHECKPOINT, which remount may set in the meantime,
>    since freeze_super() fails only when the fs is still dirty,
> - keep the check only as a WARN_ON_ONCE() safety net.
> 
> Also exclude the alias/resize reservations when updating
> current_reserved_blocks via sysfs, otherwise it can underflow
> __get_available_block_count().
> 
> Suggested-by: Chao Yu <chao@kernel.org>
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
> Changes in v2:
> - Exclude alias/resize reservations from current_reserved_blocks.
> - Allow root reserved blocks in the second check, with WARN_ON_ONCE().
> - Recheck DISABLE_CHECKPOINT after freeze_super().
> ---
>   fs/f2fs/f2fs.h  | 18 +++++++++++++----
>   fs/f2fs/gc.c    | 54 ++++++++++++++++++++++++++++++++++++++-----------
>   fs/f2fs/super.c |  1 +
>   fs/f2fs/sysfs.c | 12 ++++++++++-
>   4 files changed, 68 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 089a62c054e..6b9e1d0e71c 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1897,6 +1897,7 @@ struct f2fs_sb_info {
>   	block_t reserved_blocks;		/* configurable reserved blocks */
>   	block_t current_reserved_blocks;	/* current reserved blocks */
>   	block_t alias_reserved_blocks;		/* reserved blocks for device alias */
> +	block_t resize_reserved_blocks;		/* reserved blocks for resize_fs */
>   
>   	/* Additional tracking for no checkpoint mode */
>   	block_t unusable_block_count;		/* # of blocks saved by last cp */
> @@ -2666,16 +2667,18 @@ static inline bool __allow_reserved_root(struct f2fs_sb_info *sbi,
>   	return false;
>   }
>   
> -static inline unsigned int get_available_block_count(struct f2fs_sb_info *sbi,
> -						struct inode *inode, bool cap)
> +static inline unsigned int __get_available_block_count(
> +					struct f2fs_sb_info *sbi,
> +					bool allow_root)
>   {
>   	block_t avail_user_block_count;
>   
>   	avail_user_block_count = sbi->user_block_count -
>   					sbi->current_reserved_blocks -
> -					sbi->alias_reserved_blocks;
> +					sbi->alias_reserved_blocks -
> +					sbi->resize_reserved_blocks;

Can you please take a look at comments from sashiko? it seems valid:

https://sashiko.dev/#/patchset/20261022165526.1539629-1-daeho43%40gmail.com

__get_available_block_count() can underflow here: avail in __sbi_store() doesn't
exclude root_reserved_blocks, so once valid_user_blocks() < root_reserved_blocks,
current_reserved_blocks can overlap with root reserved space, and the unguarded
"avail_user_block_count -= root_reserved_blocks" wraps block_t.

e.g.
user_block_count = 1000, root_reserved_blocks = 100, valid = 30,
reserved_blocks = current_reserved_blocks = alias_reserved_blocks = 0

Thread A			Thread B (root)
- f2fs_resize_fs(shrunk_blocks = 500)
  - 500 + 30 <= avail(false) = 900, ok
  - resize_reserved_blocks = 500
  - drop gc_lock/cp lock
				- echo 500 > /sys/fs/f2fs/<dev>/reserved_blocks
				 - 500 <= 1000 - 100, ok
				 - avail = 1000 - 500 = 500
				 - current_reserved_blocks =
				   min(500, 500 - 30) = 470

Now for a non-root user:
__get_available_block_count(sbi, false)
   = 1000 - 470 - 0 - 500 = 30, then 30 - 100 => 0xFFFFFFBA

Thanks,

>   
> -	if (test_opt(sbi, RESERVE_ROOT) && !__allow_reserved_root(sbi, inode, cap))
> +	if (test_opt(sbi, RESERVE_ROOT) && !allow_root)
>   		avail_user_block_count -= F2FS_OPTION(sbi).root_reserved_blocks;
>   
>   	if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
> @@ -2688,6 +2691,13 @@ static inline unsigned int get_available_block_count(struct f2fs_sb_info *sbi,
>   	return avail_user_block_count;
>   }
>   
> +static inline unsigned int get_available_block_count(struct f2fs_sb_info *sbi,
> +						struct inode *inode, bool cap)
> +{
> +	return __get_available_block_count(sbi,
> +				__allow_reserved_root(sbi, inode, cap));
> +}
> +
>   static inline void f2fs_i_blocks_write(struct inode *, block_t, bool, bool);
>   static inline int inc_valid_block_count(struct f2fs_sb_info *sbi,
>   				 struct inode *inode, blkcnt_t *count,
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 926e6976bc2..955224f4166 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -2374,6 +2374,7 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
>   	struct f2fs_lock_context glc;
>   	struct f2fs_lock_context clc;
>   	unsigned int secs;
> +	bool reserved = false;
>   	int err = 0;
>   	__u32 rem;
>   
> @@ -2425,10 +2426,17 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
>   	f2fs_lock_op(sbi, &lc);
>   
>   	spin_lock(&sbi->stat_lock);
> -	if (shrunk_blocks + valid_user_blocks(sbi) +
> -		sbi->current_reserved_blocks + sbi->unusable_block_count +
> -		F2FS_OPTION(sbi).root_reserved_blocks > sbi->user_block_count)
> +	if (shrunk_blocks + valid_user_blocks(sbi) >
> +			__get_available_block_count(sbi, false)) {
>   		err = -ENOSPC;
> +	} else {
> +		/*
> +		 * Reserve the space to be shrunk so that no one, including
> +		 * root, can allocate it until the shrink is done.
> +		 */
> +		sbi->resize_reserved_blocks += shrunk_blocks;
> +		reserved = true;
> +	}
>   	spin_unlock(&sbi->stat_lock);
>   
>   	if (err)
> @@ -2442,29 +2450,45 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
>   out_drop_write:
>   	mnt_drop_write_file(filp);
>   	if (err)
> -		return err;
> +		goto out_release;
>   
>   	err = freeze_super(sbi->sb, FREEZE_HOLDER_KERNEL, NULL);
>   	if (err)
> -		return err;
> +		goto out_release;
>   
>   	if (f2fs_readonly(sbi->sb)) {
>   		err = thaw_super(sbi->sb, FREEZE_HOLDER_KERNEL, NULL);
> -		if (err)
> -			return err;
> -		return -EROFS;
> +		if (!err)
> +			err = -EROFS;
> +		goto out_release;
> +	}
> +
> +	/* checkpoint can be disabled by remount before freeze_super() */
> +	if (test_opt(sbi, DISABLE_CHECKPOINT)) {
> +		f2fs_err(sbi, "Checkpoint should be enabled.");
> +		err = thaw_super(sbi->sb, FREEZE_HOLDER_KERNEL, NULL);
> +		if (!err)
> +			err = -EINVAL;
> +		goto out_release;
>   	}
>   
>   	f2fs_down_write_trace(&sbi->gc_lock, &glc);
>   	f2fs_down_write_trace(&sbi->cp_global_sem, &clc);
>   
>   	spin_lock(&sbi->stat_lock);
> -	if (shrunk_blocks + valid_user_blocks(sbi) +
> -		sbi->current_reserved_blocks + sbi->unusable_block_count +
> -		F2FS_OPTION(sbi).root_reserved_blocks > sbi->user_block_count)
> +	/*
> +	 * shrunk_blocks is already accounted in resize_reserved_blocks, so
> +	 * this should not fail. Allow root reserved blocks here since root
> +	 * may have used them after the first check, regardless of resize.
> +	 */
> +	if (WARN_ON_ONCE(valid_user_blocks(sbi) >
> +			__get_available_block_count(sbi, true))) {
>   		err = -ENOSPC;
> -	else
> +	} else {
>   		sbi->user_block_count -= shrunk_blocks;
> +		sbi->resize_reserved_blocks -= shrunk_blocks;
> +		reserved = false;
> +	}
>   	spin_unlock(&sbi->stat_lock);
>   	if (err)
>   		goto out_err;
> @@ -2510,5 +2534,11 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
>   	f2fs_up_write_trace(&sbi->cp_global_sem, &clc);
>   	f2fs_up_write_trace(&sbi->gc_lock, &glc);
>   	thaw_super(sbi->sb, FREEZE_HOLDER_KERNEL, NULL);
> +out_release:
> +	if (reserved) {
> +		spin_lock(&sbi->stat_lock);
> +		sbi->resize_reserved_blocks -= shrunk_blocks;
> +		spin_unlock(&sbi->stat_lock);
> +	}
>   	return err;
>   }
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index d683240040f..28e49e8d71e 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -5345,6 +5345,7 @@ static int f2fs_fill_super(struct super_block *sb, struct fs_context *fc)
>   	sbi->reserved_blocks = 0;
>   	sbi->current_reserved_blocks = 0;
>   	sbi->alias_reserved_blocks = 0;
> +	sbi->resize_reserved_blocks = 0;
>   	limit_reserve_root(sbi);
>   	adjust_unusable_cap_perc(sbi);
>   
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index 9749da70089..ce70d81eba3 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -594,6 +594,8 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>   	}
>   #endif
>   	if (a->struct_type == RESERVED_BLOCKS) {
> +		block_t avail, valid;
> +
>   		spin_lock(&sbi->stat_lock);
>   		if (t > (unsigned long)(sbi->user_block_count -
>   				F2FS_OPTION(sbi).root_reserved_blocks)) {
> @@ -601,8 +603,16 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>   			return -EINVAL;
>   		}
>   		*ui = t;
> +		/*
> +		 * Exclude the space reserved for device aliasing and resize_fs,
> +		 * otherwise the reserved blocks can overlap with them and
> +		 * __get_available_block_count() can underflow.
> +		 */
> +		avail = sbi->user_block_count - sbi->alias_reserved_blocks -
> +					sbi->resize_reserved_blocks;
> +		valid = valid_user_blocks(sbi);
>   		sbi->current_reserved_blocks = min(sbi->reserved_blocks,
> -				sbi->user_block_count - valid_user_blocks(sbi));
> +				avail > valid ? avail - valid : 0);
>   		spin_unlock(&sbi->stat_lock);
>   		return count;
>   	}