Re: [RFC PATCH 4/8] fat: clean stale bdev buffers of newly allocated clusters

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

Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br> writes:

> Directory I/O uses buffer_heads on the block device, which may leave
> stale buffers behind when clusters are freed and reused. The buffer_head
> I/O paths currently call clean_bdev_aliases() for newly allocated blocks,
> preventing stale buffers from overwriting new data.
>
> Buffered iomap I/O does not provide this protection. In preparation for
> the buffered iomap conversion, clean bdev aliases on newly allocated
> clusters, like exfat patch "exfat: clean stale bdev buffers of newly
> allocated clusters".
>
> Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
> ---
>  fs/fat/fatent.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
> index 4fa1fae5abf9..46899efa3cf8 100644
> --- a/fs/fat/fatent.c
> +++ b/fs/fat/fatent.c
> @@ -523,6 +523,21 @@ static int fat_flush_bhs(struct super_block *sb, struct buffer_head **bhs,
>  	return 0;
>  }
>  
> +/*
> + * Newly allocated clusters may still have dirty buffer_heads in the block
> + * device mapping, e.g. the dentries of a directory that was just removed.
> + * File data does not go through those buffers, so drop them; otherwise a
> + * later flush of the block device writes them over the new data.
> + */
> +static void fat_clean_bdev_aliases(struct super_block *sb,
> +				   unsigned int clu, unsigned int count)
> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
> +	clean_bdev_aliases(sb->s_bdev, fat_clus_to_blknr(sbi, clu),
> +			   (sector_t)count * sbi->sec_per_clus);
> +}
> +
>  static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluster,
>  				bool return_all)
>  {
> @@ -531,7 +546,7 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
>  	const struct fatent_operations *ops = sbi->fatent_ops;
>  	struct fat_entry fatent, prev_ent;
>  	struct buffer_head *bhs[MAX_BUF_PER_PAGE];
> -	int i, count, err, nr_bhs, idx_clus;
> +	int i, count, err, nr_bhs, idx_clus, cont_start, cont_len;
>  
>  	lock_fat(sbi);
>  	if (sbi->free_clusters != -1 && sbi->free_clus_valid &&
> @@ -540,14 +555,20 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
>  		return -ENOSPC;
>  	}
>  
> +	cont_len = 0;
>  	err = nr_bhs = idx_clus = 0;
>  	count = FAT_START_ENT;
>  	fatent_init(&prev_ent);
>  	fatent_init(&fatent);
>  	fatent_set_entry(&fatent, sbi->prev_free + 1);
>  	while (count < sbi->max_cluster) {
> -		if (fatent.entry >= sbi->max_cluster)
> +		if (fatent.entry >= sbi->max_cluster) {
> +			if (cont_len) {
> +				fat_clean_bdev_aliases(sb, cont_start, cont_len);
> +				cont_len = 0;
> +			}
>  			fatent.entry = FAT_START_ENT;
> +		}
>  		fatent_set_entry(&fatent, fatent.entry);
>  		err = fat_ent_read_block(sb, &fatent);
>  		if (err)
> @@ -573,6 +594,10 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
>  					*cluster = entry;
>  				idx_clus++;
>  
> +				if (cont_len == 0)
> +					cont_start = entry;
> +				cont_len++;
> +
>  				if (nr_bhs + fatent.nr_bhs > MAX_BUF_PER_PAGE) {
>  					err = fat_flush_bhs(sb, bhs, &nr_bhs,
>  							    inode_needs_sync(inode));
> @@ -589,6 +614,9 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
>  				 * so we can still use the prev_ent.
>  				 */
>  				prev_ent = fatent;
> +			} else if (cont_len) {
> +				fat_clean_bdev_aliases(sb, cont_start, cont_len);
> +				cont_len = 0;
>  			}
>  			count++;
>  			if (count == sbi->max_cluster)
> @@ -602,6 +630,8 @@ static int __fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluste
>  	err = -ENOSPC;
>  
>  out:
> +	if (cont_len)
> +		fat_clean_bdev_aliases(sb, cont_start, cont_len);

We would want to make as possible as shorter lock_fat. At least, this
part can move outside of unlock_fat(sbi)?

>  	unlock_fat(sbi);
>  	mark_fsinfo_dirty(sb);
>  	fatent_brelse(&fatent);

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