Re: [PATCH] fat: reclaim unused KEEP_SIZE clusters on last writer release

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Yang Wen <anmuxixixi@gmail.com> writes:

> FAT releases unused clusters allocated by FALLOC_FL_KEEP_SIZE when the
> inode is evicted.  As a result, inode reclaim can enter the FAT cluster
> freeing path directly from kswapd.
>
> If another thread holds the FAT lock for a long time, kswapd can block
> while reclaiming an inode which still has unused KEEP_SIZE clusters:
>
>     Thread-B                            kswapd
>     --------                            ------
>     lock_fat()
>       mutex_lock(&sbi->fat_lock)
>                                         shrink_node()
>                                           shrink_slab()
>                                             super_cache_scan()
>                                               prune_icache_sb()
>                                                 evict()
>                                                   fat_evict_inode()
>                                                     fat_free_eofblocks()
>                                                       fat_truncate_blocks()
>                                                         fat_free_clusters()
>                                                           lock_fat()
>                                                             mutex_lock(
>                                                               &sbi->fat_lock)
>                                                             <blocked>
>
>     <holds sbi->fat_lock for a long time>
>
> This leaves kswapd blocked until Thread-B releases fat_lock and can delay
> memory reclaim for an extended period.
>
> KEEP_SIZE preallocation on FAT is not persistent.  The unused clusters
> only need to remain allocated while a writable file description can still
> consume them.  Track writable file descriptions with a counter in the FAT
> private inode.
>
> When the counter reaches zero, acquire inode->i_rwsem and reclaim the
> unused clusters if no writer has appeared in the meantime.  A writer
> opened after that check serializes subsequent write and fallocate
> operations with the cleanup through inode->i_rwsem and, as documented,
> must re-establish any reservation after reopening the file.
>
> On the last writer release, take internal freeze protection before
> inode->i_rwsem, wait for direct I/O and run the existing EOF-block cleanup
> under truncate_lock.  This keeps the metadata update outside a frozen
> filesystem without requiring a separate workqueue.
>
> This can move a fat_lock wait to the final writable file release, but
> prevents kswapd from executing the potentially blocking cluster-chain
> update directly.  Keep the cleanup in fat_evict_inode() as a fallback for
> any inode which still reaches eviction with unused preallocated clusters.

Looks like this forcibly release preallocated clusters at final write
close() even if unnecessary.

Instead, can't we penalize the side of long holding the lock (Thread-B)?

Thanks.

> Signed-off-by: Yang Wen <anmuxixixi@gmail.com>
> ---
>  fs/fat/fat.h   |  5 +++++
>  fs/fat/file.c  | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/fat/inode.c | 22 ++++++++++++++++------
>  3 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 61338413d9f3..c8bdbbaa5a0b 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -121,6 +121,9 @@ struct msdos_inode_info {
>  	/* for avoiding the race between fat_free() and fat_get_cluster() */
>  	unsigned int cache_valid_id;
>  
> +	/* Number of writable struct file instances. */
> +	atomic_t write_open_count;
> +
>  	/* NOTE: mmu_private is 64bits, so must hold ->i_mutex to access */
>  	loff_t mmu_private;	/* physically allocated size */
>  
> @@ -407,6 +410,8 @@ extern const struct inode_operations fat_file_inode_operations;
>  extern int fat_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  		       struct iattr *attr);
>  extern void fat_truncate_blocks(struct inode *inode, loff_t offset);
> +extern bool fat_has_eofblocks(struct inode *inode);
> +extern void fat_free_eofblocks(struct inode *inode);
>  extern int fat_getattr(struct mnt_idmap *idmap,
>  		       const struct path *path, struct kstat *stat,
>  		       u32 request_mask, unsigned int flags);
> diff --git a/fs/fat/file.c b/fs/fat/file.c
> index 1c835ca5f21a..935aa8e90eea 100644
> --- a/fs/fat/file.c
> +++ b/fs/fat/file.c
> @@ -173,8 +173,53 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	}
>  }
>  
> +static int fat_file_open(struct inode *inode, struct file *filp)
> +{
> +	if (filp->f_mode & FMODE_WRITER)
> +		atomic_inc(&MSDOS_I(inode)->write_open_count);
> +
> +	return 0;
> +}
> +
>  static int fat_file_release(struct inode *inode, struct file *filp)
>  {
> +	struct msdos_inode_info *ei = MSDOS_I(inode);
> +	struct super_block *sb = inode->i_sb;
> +
> +	if (filp->f_mode & FMODE_WRITER) {
> +		int writers = atomic_dec_return(&ei->write_open_count);
> +
> +		if (WARN_ON_ONCE(writers < 0)) {
> +			atomic_inc(&ei->write_open_count);
> +			goto flush;
> +		}
> +
> +		if (!writers) {
> +			sb_start_intwrite(sb);
> +			inode_lock(inode);
> +
> +			/*
> +			 * A writer which appeared before i_rwsem was acquired
> +			 * keeps the reservation alive.  A writer opened after
> +			 * this check has its write and fallocate operations
> +			 * serialized with the cleanup by i_rwsem and must
> +			 * re-establish the reservation after reopening.
> +			 */
> +			if (!atomic_read(&ei->write_open_count) &&
> +			    inode->i_nlink && fat_has_eofblocks(inode)) {
> +				inode_dio_wait(inode);
> +
> +				down_write(&ei->truncate_lock);
> +				mmb_sync(&ei->i_metadata_bhs);
> +				fat_free_eofblocks(inode);
> +				up_write(&ei->truncate_lock);
> +			}
> +
> +			inode_unlock(inode);
> +			sb_end_intwrite(sb);
> +		}
> +	}
> +flush:
>  	if ((filp->f_mode & FMODE_WRITE) &&
>  	    MSDOS_SB(inode->i_sb)->options.flush) {
>  		fat_flush_inodes(inode->i_sb, inode, NULL);
> @@ -207,6 +252,7 @@ const struct file_operations fat_file_operations = {
>  	.read_iter	= generic_file_read_iter,
>  	.write_iter	= generic_file_write_iter,
>  	.mmap_prepare	= generic_file_mmap_prepare,
> +	.open		= fat_file_open,
>  	.release	= fat_file_release,
>  	.unlocked_ioctl	= fat_generic_ioctl,
>  	.compat_ioctl	= compat_ptr_ioctl,
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index f775a004cae1..c01de4263bf9 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -659,22 +659,31 @@ static int fat_sync_inode_metadata(struct inode *inode,
>  	return mmb_sync(&MSDOS_I(inode)->i_metadata_bhs);
>  }
>  
> -static void fat_free_eofblocks(struct inode *inode)
> +bool fat_has_eofblocks(struct inode *inode)
> +{
> +	struct msdos_inode_info *ei = MSDOS_I(inode);
> +	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
> +	u64 allocated_bytes = (u64)inode->i_blocks << 9;
> +	u64 valid_bytes = round_up((u64)ei->mmu_private,
> +				   (u64)sbi->cluster_size);
> +
> +	return allocated_bytes > valid_bytes;
> +}
> +
> +void fat_free_eofblocks(struct inode *inode)
>  {
>  	/* Release unwritten fallocated blocks on inode eviction. */
> -	if ((inode->i_blocks << 9) >
> -			round_up(MSDOS_I(inode)->mmu_private,
> -				MSDOS_SB(inode->i_sb)->cluster_size)) {
> +	if (fat_has_eofblocks(inode)) {
>  		int err;
>  
>  		fat_truncate_blocks(inode, MSDOS_I(inode)->mmu_private);
>  		/* Fallocate results in updating the i_start/iogstart
>  		 * for the zero byte file. So, make it return to
> -		 * original state during evict and commit it to avoid
> +		 * * original state during cleanup and commit it to avoid
>  		 * any corruption on the next access to the cluster
>  		 * chain for the file.
>  		 */
> -		err = sync_inode_metadata(inode, inode_needs_sync(inode));
> +		err = __fat_write_inode(inode, inode_needs_sync(inode));
>  		if (err) {
>  			fat_msg(inode->i_sb, KERN_WARNING, "Failed to "
>  					"update on disk inode for unused "
> @@ -789,6 +798,7 @@ static struct inode *fat_alloc_inode(struct super_block *sb)
>  		return NULL;
>  
>  	init_rwsem(&ei->truncate_lock);
> +	atomic_set(&ei->write_open_count, 0);
>  	/* Zeroing to allow iput() even if partial initialized inode. */
>  	ei->mmu_private = 0;
>  	ei->i_start = 0;

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>