| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <055a01d63306$82b13440$88139cc0$@samsung.com>
Date: Tue, 26 May 2020 11:36:50 +0900
From: "Sungjong Seo" <sj1557.seo@...sung.com>
To: "'Tetsuhiro Kohada'" <kohada.tetsuhiro@...mitsubishielectric.co.jp>
Cc: <mori.takahiro@...mitsubishielectric.co.jp>,
<motai.hirotaka@...mitsubishielectric.co.jp>,
"'Namjae Jeon'" <namjae.jeon@...sung.com>,
<linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] exfat: optimize dir-cache
> 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@...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
Powered by blists - more mailing lists