Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Sungjong Seo" <sj1557.seo@samsung.com>
To:     "'Tetsuhiro Kohada'" <kohada.tetsuhiro@dc.mitsubishielectric.co.jp>
Cc:     <mori.takahiro@ab.mitsubishielectric.co.jp>,
        <motai.hirotaka@aj.mitsubishielectric.co.jp>,
        "'Namjae Jeon'" <namjae.jeon@samsung.com>,
        <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
In-Reply-To: <20200520075641.32441-1-kohada.tetsuhiro@dc.mitsubishielectric.co.jp>
Subject: RE: [PATCH] exfat: optimize dir-cache
Date:   Tue, 26 May 2020 11:36:50 +0900
Message-ID: <055a01d63306$82b13440$88139cc0$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Thread-Index: AQKbEd8GOCYeHYQBe/Vm0kX/gmqw5wMD/7F2pxe1QEA=
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <CGME20200520075735epcas1p269372d222e25f3fd51b7979f5b7cdc61@epcas1p2.samsung.com>
        <20200520075641.32441-1-kohada.tetsuhiro@dc.mitsubishielectric.co.jp>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> Optimize directory access based on exfat_entry_set_cache.
>  - Hold bh instead of copied d-entry.
>  - Modify bh->data directly instead of the copied d-entry.
>  - Write back the retained bh instead of rescanning the d-entry-set.
> And
>  - Remove unused cache related definitions.
> 
> Signed-off-by: Tetsuhiro Kohada
> <kohada.tetsuhiro@dc.mitsubishielectric.co.jp>
> ---
>  fs/exfat/dir.c      | 197 +++++++++++++++++---------------------------
>  fs/exfat/exfat_fs.h |  27 +++---
>  fs/exfat/file.c     |  15 ++--
>  fs/exfat/inode.c    |  53 +++++-------
>  fs/exfat/namei.c    |  14 ++--
>  5 files changed, 124 insertions(+), 182 deletions(-)
[snip]
> 
> -	es->entries[0].dentry.file.checksum = cpu_to_le16(chksum);
> +void exfat_free_dentry_set(struct exfat_entry_set_cache *es, int sync)
> +{
> +	int i;
> 
> -	while (num_entries) {
> -		/* write per sector base */
> -		remaining_byte_in_sector = (1 << sb->s_blocksize_bits) -
off;
> -		copy_entries = min_t(int,
> -			EXFAT_B_TO_DEN(remaining_byte_in_sector),
> -			num_entries);
> -		bh = sb_bread(sb, sec);
> -		if (!bh)
> -			goto err_out;
> -		memcpy(bh->b_data + off,
> -			(unsigned char *)&es->entries[0] + buf_off,
> -			EXFAT_DEN_TO_B(copy_entries));
> -		exfat_update_bh(sb, bh, sync);
> -		brelse(bh);
> -		num_entries -= copy_entries;
> -
> -		if (num_entries) {
> -			/* get next sector */
> -			if (exfat_is_last_sector_in_cluster(sbi, sec)) {
> -				clu = exfat_sector_to_cluster(sbi, sec);
> -				if (es->alloc_flag == ALLOC_NO_FAT_CHAIN)
> -					clu++;
> -				else if (exfat_get_next_cluster(sb, &clu))
> -					goto err_out;
> -				sec = exfat_cluster_to_sector(sbi, clu);
> -			} else {
> -				sec++;
> -			}
> -			off = 0;
> -			buf_off += EXFAT_DEN_TO_B(copy_entries);
> -		}
> +	for (i = 0; i < es->num_bh; i++) {
> +		if (es->modified)
> +			exfat_update_bh(es->sb, es->bh[i], sync);

Overall, it looks good to me.
However, if "sync" is set, it looks better to return the result of
exfat_update_bh().
Of course, a tiny modification for exfat_update_bh() is also required.

> +		brelse(es->bh[i]);
>  	}
> -
> -	return 0;
> -err_out:
> -	return -EIO;
> +	kfree(es);
>  }
> 
>  static int exfat_walk_fat_chain(struct super_block *sb, @@ -820,34
> +786,40 @@ static bool exfat_validate_entry(unsigned int type,
>  	}
>  }
> 
> +struct exfat_dentry *exfat_get_dentry_cached(
> +	struct exfat_entry_set_cache *es, int num) {
> +	int off = es->start_off + num * DENTRY_SIZE;
> +	struct buffer_head *bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> +	char *p = bh->b_data + EXFAT_BLK_OFFSET(off, es->sb);

In order to prevent illegal accesses to bh and dentries, it would be better
to check validation for num and bh.

> +
> +	return (struct exfat_dentry *)p;
> +}
> +
>  /*
>   * Returns a set of dentries for a file or dir.
>   *
> - * Note that this is a copy (dump) of dentries so that user should
> - * call write_entry_set() to apply changes made in this entry set
> - * to the real device.
> + * Note It provides a direct pointer to bh->data via
> exfat_get_dentry_cached().
> + * User should call exfat_get_dentry_set() after setting 'modified' to
> + apply
> + * changes made in this entry set to the real device.
>   *
>   * in:
[snip]
>  	/* check if the given file ID is opened */ @@ -153,12 +151,15 @@
> int __exfat_truncate(struct inode *inode, loff_t new_size)
>  	/* update the directory entry */
>  	if (!evict) {
>  		struct timespec64 ts;
> +		struct exfat_dentry *ep, *ep2;
> +		struct exfat_entry_set_cache *es;
> 
>  		es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
> -				ES_ALL_ENTRIES, &ep);
> +				ES_ALL_ENTRIES);
>  		if (!es)
>  			return -EIO;
> -		ep2 = ep + 1;
> +		ep = exfat_get_dentry_cached(es, 0);
> +		ep2 = exfat_get_dentry_cached(es, 1);
> 
>  		ts = current_time(inode);
>  		exfat_set_entry_time(sbi, &ts,
> @@ -185,10 +186,8 @@ int __exfat_truncate(struct inode *inode, loff_t
> new_size)
>  			ep2->dentry.stream.start_clu = EXFAT_FREE_CLUSTER;
>  		}
> 
> -		if (exfat_update_dir_chksum_with_entry_set(sb, es,
> -		    inode_needs_sync(inode)))
> -			return -EIO;
> -		kfree(es);
> +		exfat_update_dir_chksum_with_entry_set(es);
> +		exfat_free_dentry_set(es, inode_needs_sync(inode));

It would be better to return the result of exfat_free_dentry_set().

>  	}
> 
>  	/* cut off from the FAT chain */
> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c index
> 3f367d081cd6..ef7cf7a6d187 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -19,7 +19,6 @@
> 
>  static int __exfat_write_inode(struct inode *inode, int sync)  {
> -	int ret = -EIO;
>  	unsigned long long on_disk_size;
>  	struct exfat_dentry *ep, *ep2;
>  	struct exfat_entry_set_cache *es = NULL; @@ -43,11 +42,11 @@ static
> int __exfat_write_inode(struct inode *inode, int sync)
>  	exfat_set_vol_flags(sb, VOL_DIRTY);
> 
>  	/* get the directory entry of given file or directory */
> -	es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry, ES_ALL_ENTRIES,
> -		&ep);
> +	es = exfat_get_dentry_set(sb, &(ei->dir), ei->entry,
> ES_ALL_ENTRIES);
>  	if (!es)
>  		return -EIO;
> -	ep2 = ep + 1;
> +	ep = exfat_get_dentry_cached(es, 0);
> +	ep2 = exfat_get_dentry_cached(es, 1);
> 
>  	ep->dentry.file.attr = cpu_to_le16(exfat_make_attr(inode));
> 
> @@ -77,9 +76,9 @@ static int __exfat_write_inode(struct inode *inode, int
> sync)
>  	ep2->dentry.stream.valid_size = cpu_to_le64(on_disk_size);
>  	ep2->dentry.stream.size = ep2->dentry.stream.valid_size;
> 
> -	ret = exfat_update_dir_chksum_with_entry_set(sb, es, sync);
> -	kfree(es);
> -	return ret;
> +	exfat_update_dir_chksum_with_entry_set(es);
> +	exfat_free_dentry_set(es, sync);

Ditto

> +	return 0;
>  }
> 
>  int exfat_write_inode(struct inode *inode, struct writeback_control *wbc)
> @@ -110,8 +109,6 @@ static int exfat_map_cluster(struct inode *inode,
> unsigned int clu_offset,
>  	int ret, modified = false;
>  	unsigned int last_clu;
>  	struct exfat_chain new_clu;
> -	struct exfat_dentry *ep;
> -	struct exfat_entry_set_cache *es = NULL;
>  	struct super_block *sb = inode->i_sb;
>  	struct exfat_sb_info *sbi = EXFAT_SB(sb);
>  	struct exfat_inode_info *ei = EXFAT_I(inode); @@ -222,34 +219,28 @@
> static int exfat_map_cluster(struct inode *inode, unsigned int clu_offset,
>  		num_clusters += num_to_be_allocated;
>  		*clu = new_clu.dir;
> 
[snip]
> -
> -			if (exfat_update_dir_chksum_with_entry_set(sb, es,
> -			    inode_needs_sync(inode)))
> -				return -EIO;
> -			kfree(es);
> +			ep->dentry.stream.flags = ei->flags;
> +			ep->dentry.stream.start_clu =
> +				cpu_to_le32(ei->start_clu);
> +			ep->dentry.stream.valid_size =
> +				cpu_to_le64(i_size_read(inode));
> +			ep->dentry.stream.size =
> +				ep->dentry.stream.valid_size;
> +
> +			exfat_update_dir_chksum_with_entry_set(es);
> +			exfat_free_dentry_set(es, inode_needs_sync(inode));

Ditto

> 
>  		} /* end of if != DIR_DELETED */
> 
[snip]
> --
> 2.25.0