RE: [PATCH v3] exfat: speed up iterate/lookup by fixing start point of traversing cluster chain
> When directory iterate and lookup is called, there's a buggy rewinding of > start point for traversing cluster chain to the parent directory entry's > first cluster. This caused repeated cluster chain traversing from the > first entry of the parent directory that would show worse performance if > huge amounts of files exist under the parent directory. > Fix not to rewind, make continue from currently referenced cluster and dir > entry. > > Tested with 50,000 files under single directory / 256GB sdcard, with > command "time ls -l > /dev/null", > Before : 0m08.69s real 0m00.27s user 0m05.91s system > After : 0m07.01s real 0m00.25s user 0m04.34s system > > Signed-off-by: Hyeongseok Kim Looks good. Thanks for your contribution. Reviewed-by: Sungjong Seo > --- > fs/exfat/dir.c | 19 +-- > fs/exfat/exfat_fs.h | 2 +- > fs/exfat/namei.c| 9 - > 3 files changed, 22 insertions(+), 8 deletions(-)
RE: [PATCH v2] exfat: speed up iterate/lookup by fixing start point of traversing cluster chain
> When directory iterate and lookup is called, there's a buggy rewinding of
> start point for traversing cluster chain to the parent directory entry's
> first cluster. This caused repeated cluster chain traversing from the
> first entry of the parent directory that would show worse performance if
> huge amounts of files exist under the parent directory.
> Fix not to rewind, make continue from currently referenced cluster and dir
> entry.
>
> Tested with 50,000 files under single directory / 256GB sdcard, with
> command "time ls -l > /dev/null",
> Before : 0m08.69s real 0m00.27s user 0m05.91s system
> After : 0m07.01s real 0m00.25s user 0m04.34s system
>
> Signed-off-by: Hyeongseok Kim
> ---
> fs/exfat/dir.c | 39 ---
> fs/exfat/exfat_fs.h | 2 +-
> fs/exfat/namei.c| 6 --
> 3 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> e1d5536de948..63f08987a8fe 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -147,7 +147,7 @@ static int exfat_readdir(struct inode *inode, loff_t
> *cpos, struct exfat_dir_ent
[snip]
> + * @de: If p_uniname is found, filled with optimized dir/entry
> + * for traversing cluster chain. Basically,
> + * (p_dir.dir+return entry) and (de.dir.dir+de.entry) are
> + * pointing the same physical directory entry, but if
> + * caller needs to start to traverse cluster chain,
> + * it's better option to choose the information in de.
> + * Caller could only trust .dir and .entry field.
exfat-fs has exfat_hint structure for keeping clusters and entries as hints.
Of course, the caller, exfat_find(), should adjust exfat_chain with
hint value just before calling exfat_get_dentry_set() as follows.
/* adjust cdir to the optimized value */
cdir.dir = hint_opt.clu;
if (cdir.flag & ALLOC_NO_FAT_CHAIN) {
cdir.size -= dentry / sbi->dentries_per_clu;
dentry = hint_opt.eidx;
What do you think about using it?
> + * @return:
> + * >= 0: file directory entry position where the name exists
> + * -ENOENT: entry with the name does not exist
> + * -EIO: I/O error
> */
[snip]
> @@ -1070,11 +1081,14 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei,
> }
>
> if (clu.flags == ALLOC_NO_FAT_CHAIN) {
> - if (--clu.size > 0)
> + if (--clu.size > 0) {
> + exfat_chain_dup(>dir, );
If you want to make a backup of the clu, it seems more appropriate to move
exfat_chain_dup() right above the "if".
> clu.dir++;
> + }
> else
> clu.dir = EXFAT_EOF_CLUSTER;
> } else {
> + exfat_chain_dup(>dir, );
> if (exfat_get_next_cluster(sb, ))
> return -EIO;
> }
> @@ -1101,6 +1115,17 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei,
> return -ENOENT;
>
> found:
> + /* set as dentry in cluster */
> + de->entry = (dentry - num_ext) & (dentries_per_clu - 1);
> + /*
> + * if dentry_set spans to the next_cluster,
> + * e.g. (de->entry + num_ext + 1 > dentries_per_clu)
> + * current de->dir is correct which have previous cluster info,
> + * but if it doesn't span as below, "clu" is correct, so update.
> + */
> + if (de->entry + num_ext + 1 <= dentries_per_clu)
> + exfat_chain_dup(>dir, );
> +
Let it be simple.
1. Keep an old value in the stack variable, when it found a FILE or DIR
entry.
2. And just copy that here.
There are more assignments, but I think its impact might be negligible.
Thanks.
RE: [PATCH] exfat: speed up iterate/lookup by fixing start point of traversing fat chain
> When directory iterate and lookup is called, there is a buggy rewinding of
> start point for traversing fat chain to the directory entry's first
> cluster. This caused repeated fat chain traversing from the first entry of
> the directory that would show worse performance if huge amounts of files
> exist under single directory.
> Fix not to rewind, make continue from currently referenced cluster and dir
> entry.
>
> Tested with 50,000 files under single directory / 256GB sdcard, with
> command "time ls -l > /dev/null",
> Before : 0m08.69s real 0m00.27s user 0m05.91s system
> After : 0m07.01s real 0m00.25s user 0m04.34s system
>
> Signed-off-by: Hyeongseok Kim
> ---
> fs/exfat/dir.c | 42 +-
> 1 file changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> e1d5536de948..59d12eaa0649 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -147,7 +147,7 @@ static int exfat_readdir(struct inode *inode, loff_t
> *cpos, struct exfat_dir_ent
> 0);
>
> *uni_name.name = 0x0;
> - exfat_get_uniname_from_ext_entry(sb, , dentry,
> + exfat_get_uniname_from_ext_entry(sb, , i,
> uni_name.name);
Looks good. Old code looks like a bug as you said.
> exfat_utf16_to_nls(sb, _name,
> dir_entry->namebuf.lfn,
> @@ -911,10 +911,15 @@ enum {
> };
>
> /*
> - * return values:
> - * >= 0: return dir entiry position with the name in dir
> - * -ENOENT : entry with the name does not exist
> - * -EIO: I/O error
> + * @ei: inode info of directory
> + * @p_dir: input as directory structure in which we search name
> + * if found, output as a cluster dir where the name exists
> + * if not found, not changed from input
> + * @num_entries entry size of p_uniname
> + * @return:
> + * >= 0: dir entry position from output p_dir.dir
> + * -ENOENT: entry with the name does not exist
> + * -EIO: I/O error
> */
> int exfat_find_dir_entry(struct super_block *sb, struct exfat_inode_info
> *ei,
> struct exfat_chain *p_dir, struct exfat_uni_name *p_uniname,
> @@ -925,14 +930,16 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei,
[snip]
hint_stat->clu = p_dir->dir;
> hint_stat->eidx = 0;
> - return (dentry - num_ext);
> +
> + exfat_chain_dup(p_dir, _clu);
> + return dentry_in_cluster;
> }
> }
>
> hint_stat->clu = clu.dir;
> hint_stat->eidx = dentry + 1;
> - return dentry - num_ext;
> +
> + exfat_chain_dup(p_dir, _clu);
> + return dentry_in_cluster;
> }
Changing the functionality of exfat find_dir_entry() will affect
exfat_find() and exfat_lookup(), breaking the concept of ei->dir.dir
which should have the starting cluster of its parent directory.
Well, is there any missing patch related to exfat_find()?
It would be nice to modify the caller of this function, exfat_find(),
so that this change in functionality doesn't affect other functions.
Thanks.
>
> int exfat_count_ext_entries(struct super_block *sb, struct exfat_chain
> *p_dir,
> --
> 2.27.0.83.g0313f36
RE: [PATCH] exfat: improve write performance when dirsync enabled
> Degradation of write speed caused by frequent disk access for cluster > bitmap update on every cluster allocation could be improved by selective > syncing bitmap buffer. Change to flush bitmap buffer only for the > directory related operations. > > Signed-off-by: Hyeongseok Kim Looks good. Thanks for your work. Acked-by: Sungjong Seo
RE: [PATCH v4 2/2] exfat: add support ioctl and FITRIM function
> Add FITRIM ioctl to enable discarding unused blocks while mounted. > As current exFAT doesn't have generic ioctl handler, add empty ioctl > function first, and add FITRIM handler. > > Reviewed-by: Chaitanya Kulkarni > Signed-off-by: Hyeongseok Kim > --- > fs/exfat/balloc.c | 80 + > fs/exfat/dir.c | 5 +++ > fs/exfat/exfat_fs.h | 4 +++ > fs/exfat/file.c | 53 ++ > 4 files changed, 142 insertions(+) > It looks better than v3. Thanks for your work! Acked-by: Sungjong Seo BTW, there is still a problem that the alloc/free cluster operation waits until the trimfs operation is finished. Any ideas for improvement in the future are welcome. :)
RE: [PATCH v4 1/2] exfat: introduce bitmap_lock for cluster bitmap access
> s_lock which is for protecting concurrent access of file operations is too > huge for cluster bitmap protection, so introduce a new bitmap_lock to > narrow the lock range if only need to access cluster bitmap. > > Signed-off-by: Hyeongseok Kim Looks good. Thanks for your work! Acked-by: Sungjong Seo
RE: [PATCH] exfat: fix erroneous discard when clear cluster bit
> Subject: [PATCH] exfat: fix erroneous discard when clear cluster bit
>
> If mounted with discard option, exFAT issues discard command when clear
> cluster bit to remove file. But the input parameter of cluster-to-sector
> calculation is abnormally adds reserved cluster size which is 2, leading
> to discard unrelated sectors included in target+2 cluster.
>
> Fixes: 1e49a94cf707 ("exfat: add bitmap operations")
> Signed-off-by: Hyeongseok Kim
> ---
> fs/exfat/balloc.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Looks good.
Acked-by: Sungjong Seo
Thanks for your work!
Could you remove the wrong comments above set/clear/find bitmap functions
together?
RE: [PATCH] exfat: improve performance of exfat_free_cluster when using dirsync mount option
> There are stressful update of cluster allocation bitmap when using dirsync > mount option which is doing sync buffer on every cluster bit clearing. > This could result in performance degradation when deleting big size file. > Fix to update only when the bitmap buffer index is changed would make less > disk access, improving performance especially for truncate operation. > > Testing with Samsung 256GB sdcard, mounted with dirsync option (mount -t > exfat /dev/block/mmcblk0p1 /temp/mount -o dirsync) > > Remove 4GB file, blktrace result. > [Before] : 39 secs. > Total (blktrace): > Reads Queued: 0,0KiB Writes Queued: 32775, 16387KiB > Read Dispatches: 0,0KiB Write Dispatches: 32775, 16387KiB > Reads Requeued:0 Writes Requeued:0 > Reads Completed: 0,0KiB Writes Completed: 32775, 16387KiB > Read Merges: 0,0KiB Write Merges: 0, 0KiB > IO unplugs:2 Timer unplugs: 0 > > [After] : 1 sec. > Total (blktrace): > Reads Queued: 0,0KiB Writes Queued: 13, 6KiB > Read Dispatches: 0,0KiB Write Dispatches: 13, 6KiB > Reads Requeued:0 Writes Requeued:0 > Reads Completed: 0,0KiB Writes Completed: 13, 6KiB > Read Merges: 0,0KiB Write Merges: 0, 0KiB > IO unplugs:1 Timer unplugs: 0 > > Signed-off-by: Hyeongseok Kim Looks good. Thanks for your work! Acked-by: Sungjong Seo
RE: [PATCH v3] exfat: Avoid allocating upcase table using kcalloc()
> The table for Unicode upcase conversion requires an order-5 allocation,
> which may fail on a highly-fragmented system:
>
> pool-udisksd: page allocation failure: order:5,
> mode:0x40dc0(GFP_KERNEL|__GFP_COMP|__GFP_ZERO),
> nodemask=(null),cpuset=/,mems_allowed=0
> CPU: 4 PID: 3756880 Comm: pool-udisksd Tainted: G U
5.8.10-
> 200.fc32.x86_64 #1
> Hardware name: Dell Inc. XPS 13 9360/0PVG6D, BIOS 2.13.0 11/14/2019 Call
> Trace:
> dump_stack+0x6b/0x88
> warn_alloc.cold+0x75/0xd9
> ? _cond_resched+0x16/0x40
> ? __alloc_pages_direct_compact+0x144/0x150
> __alloc_pages_slowpath.constprop.0+0xcfa/0xd30
> ? __schedule+0x28a/0x840
> ? __wait_on_bit_lock+0x92/0xa0
> __alloc_pages_nodemask+0x2df/0x320
> kmalloc_order+0x1b/0x80
> kmalloc_order_trace+0x1d/0xa0
> exfat_create_upcase_table+0x115/0x390 [exfat]
> exfat_fill_super+0x3ef/0x7f0 [exfat]
> ? sget_fc+0x1d0/0x240
> ? exfat_init_fs_context+0x120/0x120 [exfat]
> get_tree_bdev+0x15c/0x250
> vfs_get_tree+0x25/0xb0
> do_mount+0x7c3/0xaf0
> ? copy_mount_options+0xab/0x180
> __x64_sys_mount+0x8e/0xd0
> do_syscall_64+0x4d/0x90
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Convert kcalloc/kfree to their kv* variants to eliminate the issue.
>
> Cc: sta...@vger.kernel.org # v5.7+
> Signed-off-by: Artem Labazov <123321art...@gmail.com>
Looks good.
Thanks for your contribution.
Reviewed-by: Sungjong Seo
> ---
> v2: replace vmalloc with vzalloc to avoid uninitialized memory access
> v3: use kv* functions to attempt kmalloc first
>
> fs/exfat/nls.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c index
> 675d0e7058c5..314d5407a1be 100644
> --- a/fs/exfat/nls.c
> +++ b/fs/exfat/nls.c
> @@ -659,7 +659,7 @@ static int exfat_load_upcase_table(struct super_block
> *sb,
> unsigned char skip = false;
> unsigned short *upcase_table;
>
> - upcase_table = kcalloc(UTBL_COUNT, sizeof(unsigned short),
> GFP_KERNEL);
> + upcase_table = kvcalloc(UTBL_COUNT, sizeof(unsigned short),
> +GFP_KERNEL);
> if (!upcase_table)
> return -ENOMEM;
>
> @@ -715,7 +715,7 @@ static int exfat_load_default_upcase_table(struct
> super_block *sb)
> unsigned short uni = 0, *upcase_table;
> unsigned int index = 0;
>
> - upcase_table = kcalloc(UTBL_COUNT, sizeof(unsigned short),
> GFP_KERNEL);
> + upcase_table = kvcalloc(UTBL_COUNT, sizeof(unsigned short),
> +GFP_KERNEL);
> if (!upcase_table)
> return -ENOMEM;
>
> @@ -803,5 +803,5 @@ int exfat_create_upcase_table(struct super_block *sb)
>
> void exfat_free_upcase_table(struct exfat_sb_info *sbi) {
> - kfree(sbi->vol_utbl);
> + kvfree(sbi->vol_utbl);
> }
> --
> 2.26.2
RE: [PATCH] exfat: Avoid allocating upcase table using kcalloc()
> > I have not yet received a report of the same issue. > > But I agree that this problem is likely to occur even if it is low > > probability. > > Perhaps I should clarify my setup a little bit more. > The issue can be reliably reproduced on my laptop. It has 8 GBs of RAM > (pretty common amount nowadays) and runs an unmodified Fedora 32 kernel. > Also, I use zswap, which seems to be contributing to fragmentation as well. > > > I think it would be more appropriate to use kvcalloc and kvfree instead. > > I do not think this is really needed. > Upcase table allocation is relatively large (32 pages of 4KB size) and > happens only once, when the drive is being mounted. Also, exfat driver > does not rely on the fact that the table is physically contiguous. > That said, vmalloc/vfree seems to be the best option, according to > kernel's "Memory Allocation Guide". The address range available for vmalloc() allocations is limited on 32-bit systems. If all kernel codes that need non-contiguous memory of the size order 1 or larger try to allocate it by only vmalloc(), the address range for vmalloc() could be insufficient. So, I think it would be better to give kmalloc() "one" chance. I know that kvmalloc() only tries kmalloc() once (noretry, nowarn) and if it fails, it immediately falls back to vmalloc(). Therefore, I think kvmalloc() and kvfree() are the best solution for solving the problem you are facing and the problem I mentioned above. Could you send me patch v3 that uses kvcalloc() and kvfree()? > > -- > Artem
RE: [PATCH] exfat: Avoid allocating upcase table using kcalloc()
> The table for Unicode upcase conversion requires an order-5 allocation, > which may fail on a highly-fragmented system: > > pool-udisksd: page allocation failure: order:5, > mode:0x40dc0(GFP_KERNEL|__GFP_COMP|__GFP_ZERO), > nodemask=(null),cpuset=/,mems_allowed=0 > CPU: 4 PID: 3756880 Comm: pool-udisksd Tainted: G U 5.8.10- > 200.fc32.x86_64 #1 > Hardware name: Dell Inc. XPS 13 9360/0PVG6D, BIOS 2.13.0 11/14/2019 Call > Trace: > dump_stack+0x6b/0x88 > warn_alloc.cold+0x75/0xd9 > ? _cond_resched+0x16/0x40 > ? __alloc_pages_direct_compact+0x144/0x150 > __alloc_pages_slowpath.constprop.0+0xcfa/0xd30 > ? __schedule+0x28a/0x840 > ? __wait_on_bit_lock+0x92/0xa0 > __alloc_pages_nodemask+0x2df/0x320 > kmalloc_order+0x1b/0x80 > kmalloc_order_trace+0x1d/0xa0 > exfat_create_upcase_table+0x115/0x390 [exfat] > exfat_fill_super+0x3ef/0x7f0 [exfat] > ? sget_fc+0x1d0/0x240 > ? exfat_init_fs_context+0x120/0x120 [exfat] > get_tree_bdev+0x15c/0x250 > vfs_get_tree+0x25/0xb0 > do_mount+0x7c3/0xaf0 > ? copy_mount_options+0xab/0x180 > __x64_sys_mount+0x8e/0xd0 > do_syscall_64+0x4d/0x90 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > > Make the driver use vmalloc() to eliminate the issue. I have not yet received a report of the same issue. But I agree that this problem is likely to occur even if it is low probability. I think it would be more appropriate to use kvcalloc and kvfree instead. Could you send me v2 patch?
RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
> >> --- a/fs/exfat/namei.c > >> +++ b/fs/exfat/namei.c > >> @@ -1095,11 +1095,6 @@ static int exfat_move_file(struct inode > >> *inode, struct exfat_chain *p_olddir, > >>if (!epmov) > >>return -EIO; > >> > >> - /* check if the source and target directory is the same */ > >> - if (exfat_get_entry_type(epmov) == TYPE_DIR && > >> - le32_to_cpu(epmov->dentry.stream.start_clu) == p_newdir->dir) > >> - return -EINVAL; > >> - > > > > It might check if the cluster numbers are same between source entry > > and target directory. > > This checks if newdir is the move target itself. > Example: >mv /mnt/dir0 /mnt/dir0/foo > > However, this check is not enough. > We need to check newdir and all ancestors. > Example: >mv /mnt/dir0 /mnt/dir0/dir1/foo >mv /mnt/dir0 /mnt/dir0/dir1/dir2/foo >... > > This is probably a taboo for all layered filesystems. > > > > Could you let me know what code you mentioned? > > Or do you mean the codes on vfs? > > You can find in do_renameat2(). --- around 'fs/namei.c:4440' > If the destination ancestors are itself, our driver will not be called. I think, of course, vfs has been doing that. So that code is unnecessary in normal situations. That code comes from the old exfat implementation. And as far as I understand, it seems to check once more "the cluster number" even though it comes through vfs so that it tries detecting abnormal of on-disk. Anyway, I agonized if it is really needed. In conclusion, old code could be eliminated and your patch looks reasonable. Thanks Acked-by: Sungjong Seo > > > BTW > Are you busy now? I'm sorry, I'm so busy for my full time work :( Anyway, I'm trying to review serious bug patches or bug reports first. Other patches, such as clean-up or code refactoring, may take some time to review. > I am waiting for your reply about "integrates dir-entry getting and > validation" patch. As I know, your patch is being under review by Namjae. > > BR > --- > Tetsuhiro Kohada
RE: [PATCH] exfat: remove 'rwoffset' in exfat_inode_info
> On 2020/09/12 14:01, Sungjong Seo wrote: > >> Remove 'rwoffset' in exfat_inode_info and replace it with the > >> parameter(cpos) of exfat_readdir. > >> Since rwoffset of is referenced only by exfat_readdir, it is not > >> necessary a exfat_inode_info's member. > >> > >> Signed-off-by: Tetsuhiro Kohada > >> --- > >> fs/exfat/dir.c | 16 ++-- > >> fs/exfat/exfat_fs.h | 2 -- > >> fs/exfat/file.c | 2 -- > >> fs/exfat/inode.c| 3 --- > >> fs/exfat/super.c| 1 - > >> 5 files changed, 6 insertions(+), 18 deletions(-) > >> > >> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index > >> a9b13ae3f325..fa5bb72aa295 100644 > >> --- a/fs/exfat/dir.c > >> +++ b/fs/exfat/dir.c > > [snip] > >> sector @@ -262,13 +260,11 @@ static int exfat_iterate(struct file > >> *filp, struct dir_context *ctx) > >>goto end_of_dir; > >>} > >> > >> - cpos = EXFAT_DEN_TO_B(ei->rwoffset); > >> - > >>if (!nb->lfn[0]) > >>goto end_of_dir; > >> > >>i_pos = ((loff_t)ei->start_clu << 32) | > >> - ((ei->rwoffset - 1) & 0x); > >> + (EXFAT_B_TO_DEN(cpos-1) & 0x); > > > > Need to fix the above line to be: > > (EXFAT_B_TO_DEN(cpos)-1)) & 0x); > > > Here, we simply converted so that the calculation results would be the > same. > But after reading it carefully again, I noticed. > - Why use the previous entry? > - Why does cpos point to stream dir-entry in entry-set? > > For the former, there is no need to "++dentry" in exfat_readdir(). > For the latter, I think cpos should point to the next to current entry-set. > > I'll make V2 considering these. > How do you think? The latter looks better. > > BR > --- > Tetsuhiro Kohada
RE: [PATCH v2] exfat: remove 'rwoffset' in exfat_inode_info
> Remove 'rwoffset' in exfat_inode_info and replace it with the parameter of
> exfat_readdir().
> Since rwoffset is referenced only by exfat_readdir(), it is not necessary
> a exfat_inode_info's member.
> Also, change cpos to point to the next of entry-set, and return the index
> of dir-entry via dir_entry->entry.
>
> Signed-off-by: Tetsuhiro Kohada
Acked-by: Sungjong Seo
> ---
> Changes in v2
> - 'cpos' point to the next of entry-set
> - return the index of dir-entry via dir_entry->entry
> - fix commit-message
>
> fs/exfat/dir.c | 21 +
> fs/exfat/exfat_fs.h | 2 --
> fs/exfat/file.c | 2 --
> fs/exfat/inode.c| 3 ---
> fs/exfat/super.c| 1 -
> 5 files changed, 9 insertions(+), 20 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> a9b13ae3f325..82bee625549d 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -59,9 +59,9 @@ static void exfat_get_uniname_from_ext_entry(struct
> super_block *sb, }
>
> /* read a directory entry from the opened directory */ -static int
> exfat_readdir(struct inode *inode, struct exfat_dir_entry *dir_entry)
> +static int exfat_readdir(struct inode *inode, loff_t *cpos, struct
> +exfat_dir_entry *dir_entry)
> {
> - int i, dentries_per_clu, dentries_per_clu_bits = 0;
> + int i, dentries_per_clu, dentries_per_clu_bits = 0, num_ext;
> unsigned int type, clu_offset;
> sector_t sector;
> struct exfat_chain dir, clu;
> @@ -70,7 +70,7 @@ static int exfat_readdir(struct inode *inode, struct
> exfat_dir_entry *dir_entry)
> struct super_block *sb = inode->i_sb;
> struct exfat_sb_info *sbi = EXFAT_SB(sb);
> struct exfat_inode_info *ei = EXFAT_I(inode);
> - unsigned int dentry = ei->rwoffset & 0x;
> + unsigned int dentry = EXFAT_B_TO_DEN(*cpos) & 0x;
> struct buffer_head *bh;
>
> /* check if the given file ID is opened */ @@ -127,6 +127,7 @@
> static int exfat_readdir(struct inode *inode, struct exfat_dir_entry
> *dir_entry)
> continue;
> }
>
> + num_ext = ep->dentry.file.num_ext;
> dir_entry->attr = le16_to_cpu(ep->dentry.file.attr);
> exfat_get_entry_time(sbi, _entry->crtime,
> ep->dentry.file.create_tz,
> @@ -157,12 +158,13 @@ static int exfat_readdir(struct inode *inode, struct
> exfat_dir_entry *dir_entry)
> return -EIO;
> dir_entry->size =
> le64_to_cpu(ep->dentry.stream.valid_size);
> + dir_entry->entry = dentry;
> brelse(bh);
>
> ei->hint_bmap.off = dentry >> dentries_per_clu_bits;
> ei->hint_bmap.clu = clu.dir;
>
> - ei->rwoffset = ++dentry;
> + *cpos = EXFAT_DEN_TO_B(dentry + 1 + num_ext);
> return 0;
> }
>
> @@ -178,7 +180,7 @@ static int exfat_readdir(struct inode *inode, struct
> exfat_dir_entry *dir_entry)
> }
>
> dir_entry->namebuf.lfn[0] = '\0';
> - ei->rwoffset = dentry;
> + *cpos = EXFAT_DEN_TO_B(dentry);
> return 0;
> }
>
> @@ -242,12 +244,10 @@ static int exfat_iterate(struct file *filp, struct
> dir_context *ctx)
> if (err)
> goto unlock;
> get_new:
> - ei->rwoffset = EXFAT_B_TO_DEN(cpos);
> -
> if (cpos >= i_size_read(inode))
> goto end_of_dir;
>
> - err = exfat_readdir(inode, );
> + err = exfat_readdir(inode, , );
> if (err) {
> /*
>* At least we tried to read a sector. Move cpos to next
> sector @@ -262,13 +262,10 @@ static int exfat_iterate(struct file *filp,
> struct dir_context *ctx)
> goto end_of_dir;
> }
>
> - cpos = EXFAT_DEN_TO_B(ei->rwoffset);
> -
> if (!nb->lfn[0])
> goto end_of_dir;
>
> - i_pos = ((loff_t)ei->start_clu << 32) |
> - ((ei->rwoffset - 1) & 0x);
> + i_pos = ((loff_t)ei->start_clu << 32) | (de.entry & 0x);
> tmp = exfat_iget(sb, i_pos);
> if (tmp) {
> inum = tmp->i_ino;
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> 44dc04520175..e586daf5a2e7 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -263,8 +263,6 @@ struct exfat_inode_info {
>* the validation of hint_stat.
RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
> In exfat_move_file(), the identity of source and target directory has been > checked by the caller. > Also, it gets stream.start_clu from file dir-entry, which is an invalid > determination. > > Signed-off-by: Tetsuhiro Kohada > --- > fs/exfat/namei.c | 5 - > 1 file changed, 5 deletions(-) > > diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index > 803748946ddb..1c433491f771 100644 > --- a/fs/exfat/namei.c > +++ b/fs/exfat/namei.c > @@ -1095,11 +1095,6 @@ static int exfat_move_file(struct inode *inode, > struct exfat_chain *p_olddir, > if (!epmov) > return -EIO; > > - /* check if the source and target directory is the same */ > - if (exfat_get_entry_type(epmov) == TYPE_DIR && > - le32_to_cpu(epmov->dentry.stream.start_clu) == p_newdir->dir) > - return -EINVAL; > - It might check if the cluster numbers are same between source entry and target directory. Could you let me know what code you mentioned? Or do you mean the codes on vfs? > num_old_entries = exfat_count_ext_entries(sb, p_olddir, oldentry, > epmov); > if (num_old_entries < 0) > -- > 2.25.1
RE: [PATCH 3/3] exfat: replace memcpy with structure assignment
> Use structure assignment instead of memcpy.
>
> Signed-off-by: Tetsuhiro Kohada
Acked-by: Sungjong Seo
> ---
> fs/exfat/dir.c | 7 ++-
> fs/exfat/inode.c | 2 +-
> fs/exfat/namei.c | 15 +++
> 3 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> fa5bb72aa295..8520decd120c 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -974,11 +974,8 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei,
> if (ei->hint_femp.eidx ==
> EXFAT_HINT_NONE ||
> candi_empty.eidx <=
> - ei->hint_femp.eidx)
{
> - memcpy(>hint_femp,
> - _empty,
> -
sizeof(candi_empty));
> - }
> + ei->hint_femp.eidx)
> + ei->hint_femp = candi_empty;
> }
>
> brelse(bh);
> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c index
> 70a33d4807c3..687f77653187 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -554,7 +554,7 @@ static int exfat_fill_inode(struct inode *inode,
> struct exfat_dir_entry *info)
> struct exfat_inode_info *ei = EXFAT_I(inode);
> loff_t size = info->size;
>
> - memcpy(>dir, >dir, sizeof(struct exfat_chain));
> + ei->dir = info->dir;
> ei->entry = info->entry;
> ei->attr = info->attr;
> ei->start_clu = info->start_clu;
> diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index
> 1c433491f771..2932b23a3b6c 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -318,8 +318,7 @@ static int exfat_find_empty_entry(struct inode *inode,
> hint_femp.eidx = EXFAT_HINT_NONE;
>
> if (ei->hint_femp.eidx != EXFAT_HINT_NONE) {
> - memcpy(_femp, >hint_femp,
> - sizeof(struct exfat_hint_femp));
> + hint_femp = ei->hint_femp;
> ei->hint_femp.eidx = EXFAT_HINT_NONE;
> }
>
> @@ -519,7 +518,7 @@ static int exfat_add_entry(struct inode *inode, const
> char *path,
> if (ret)
> goto out;
>
> - memcpy(>dir, p_dir, sizeof(struct exfat_chain));
> + info->dir = *p_dir;
> info->entry = dentry;
> info->flags = ALLOC_NO_FAT_CHAIN;
> info->type = type;
> @@ -625,7 +624,7 @@ static int exfat_find(struct inode *dir, struct qstr
> *qname,
> if (dentry < 0)
> return dentry; /* -error value */
>
> - memcpy(>dir, , sizeof(struct exfat_chain));
> + info->dir = cdir;
> info->entry = dentry;
> info->num_subdirs = 0;
>
> @@ -1030,7 +1029,7 @@ static int exfat_rename_file(struct inode *inode,
> struct exfat_chain *p_dir,
> if (!epnew)
> return -EIO;
>
> - memcpy(epnew, epold, DENTRY_SIZE);
> + *epnew = *epold;
> if (exfat_get_entry_type(epnew) == TYPE_FILE) {
> epnew->dentry.file.attr |=
cpu_to_le16(ATTR_ARCHIVE);
> ei->attr |= ATTR_ARCHIVE;
> @@ -1050,7 +1049,7 @@ static int exfat_rename_file(struct inode *inode,
> struct exfat_chain *p_dir,
> return -EIO;
> }
>
> - memcpy(epnew, epold, DENTRY_SIZE);
> + *epnew = *epold;
> exfat_update_bh(new_bh, sync);
> brelse(old_bh);
> brelse(new_bh);
> @@ -1113,7 +1112,7 @@ static int exfat_move_file(struct inode *inode,
> struct exfat_chain *p_olddir,
> if (!epnew)
> return -EIO;
>
> - memcpy(epnew, epmov, DENTRY_SIZE);
> + *epnew = *epmov;
> if (exfat_get_entry_type(epnew) == TYPE_FILE) {
> epnew->dentry.file.attr |= cpu_to_le16(ATTR_ARCHIVE);
> ei->attr |= ATTR_ARCHIVE;
> @@ -1133,7 +1132,7 @@ static int exfat_move_file(struct inode *inode,
> struct exfat_chain *p_olddir,
> return -EIO;
> }
>
> - memcpy(epnew, epmov, DENTRY_SIZE);
> + *epnew = *epmov;
> exfat_update_bh(new_bh, IS_DIRSYNC(inode));
> brelse(mov_bh);
> brelse(new_bh);
> --
> 2.25.1
RE: [PATCH 1/3] exfat: remove useless directory scan in exfat_add_entry()
> There is nothing in directory just created, so there is no need to scan.
>
> Signed-off-by: Tetsuhiro Kohada
Acked-by: Sungjong Seo
> ---
> fs/exfat/namei.c | 11 +--
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index
> b966b9120c9c..803748946ddb 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -530,19 +530,10 @@ static int exfat_add_entry(struct inode *inode,
> const char *path,
> info->size = 0;
> info->num_subdirs = 0;
> } else {
> - int count;
> - struct exfat_chain cdir;
> -
> info->attr = ATTR_SUBDIR;
> info->start_clu = start_clu;
> info->size = clu_size;
> -
> - exfat_chain_set(, info->start_clu,
> - EXFAT_B_TO_CLU(info->size, sbi), info->flags);
> - count = exfat_count_dir_entries(sb, );
> - if (count < 0)
> - return -EIO;
> - info->num_subdirs = count + EXFAT_MIN_SUBDIR;
> + info->num_subdirs = EXFAT_MIN_SUBDIR;
> }
> memset(>crtime, 0, sizeof(info->crtime));
> memset(>mtime, 0, sizeof(info->mtime));
> --
> 2.25.1
RE: [PATCH] exfat: remove 'rwoffset' in exfat_inode_info
> Remove 'rwoffset' in exfat_inode_info and replace it with the > parameter(cpos) of exfat_readdir. > Since rwoffset of is referenced only by exfat_readdir, it is not > necessary a exfat_inode_info's member. > > Signed-off-by: Tetsuhiro Kohada > --- > fs/exfat/dir.c | 16 ++-- > fs/exfat/exfat_fs.h | 2 -- > fs/exfat/file.c | 2 -- > fs/exfat/inode.c| 3 --- > fs/exfat/super.c| 1 - > 5 files changed, 6 insertions(+), 18 deletions(-) > > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index > a9b13ae3f325..fa5bb72aa295 100644 > --- a/fs/exfat/dir.c > +++ b/fs/exfat/dir.c [snip] > sector @@ -262,13 +260,11 @@ static int exfat_iterate(struct file *filp, > struct dir_context *ctx) > goto end_of_dir; > } > > - cpos = EXFAT_DEN_TO_B(ei->rwoffset); > - > if (!nb->lfn[0]) > goto end_of_dir; > > i_pos = ((loff_t)ei->start_clu << 32) | > - ((ei->rwoffset - 1) & 0x); > + (EXFAT_B_TO_DEN(cpos-1) & 0x); Need to fix the above line to be: (EXFAT_B_TO_DEN(cpos)-1)) & 0x);
RE: [PATCH] exfat: eliminate dead code in exfat_find()
> The exfat_find_dir_entry() called by exfat_find() doesn't return -EEXIST. > Therefore, the root-dir information setting is never executed. > > Signed-off-by: Tetsuhiro Kohada Acked-by: Sungjong Seo > --- > fs/exfat/dir.c | 1 - > fs/exfat/namei.c | 120 +++ > 2 files changed, 47 insertions(+), 74 deletions(-)
RE: [PATCH 2/2] exfat: unify name extraction
> Thanks for your reply.
>
> On 2020/08/09 2:19, Sungjong Seo wrote:
> > [snip]
> >> @@ -963,80 +942,38 @@ int exfat_find_dir_entry(struct super_block
> >> *sb, struct exfat_inode_info *ei,
> >>num_empty = 0;
> >>candi_empty.eidx = EXFAT_HINT_NONE;
> >>
> > [snip]
> >>
> >> - if (entry_type &
> >> - (TYPE_CRITICAL_SEC |
> > TYPE_BENIGN_SEC)) {
> >> - if (step == DIRENT_STEP_SECD) {
> >> - if (++order == num_ext)
> >> - goto found;
> >> - continue;
> >> - }
> >> + exfat_get_uniname_from_name_entries(es, _name);
> >
> > It is needed to check a return value.
>
> I'll fix it in v2.
>
>
> >> + exfat_free_dentry_set(es, false);
> >> +
> >> + if (!exfat_uniname_ncmp(sb,
> >> + p_uniname->name,
> >> + uni_name.name,
> >> + name_len)) {
> >> + /* set the last used position as hint */
> >> + hint_stat->clu = clu.dir;
> >> + hint_stat->eidx = dentry;
> >
> > eidx and clu of hint_stat should have one for the next entry we'll
> > start looking for.
> > Did you intentionally change the concept?
>
> Yes, this is intentional.
> Essentially, the "Hint" concept is to reduce the next seek cost with
> minimal cost.
> There is a difference in the position of the hint, but the concept is the
> same.
> As you can see, the patched code strategy doesn't move from current
> position.
> Basically, the original code strategy is advancing only one dentry.(It's
> the "minimum cost") However, when it reaches the cluster boundary, it gets
> the next cluster and error handling.
I didn't get exactly what "original code" is.
Do you mean whole code lines for exfat_find_dir_entry()?
Or just only for handling the hint in it?
The strategy of original code for hint is advancing not one dentry but one
dentry_set.
If a hint position is not moved to next like the patched code,
caller have to start at old dentry_set that could be already loaded on dentry
cache.
Let's think the case of searching through all files sequentially.
The patched code should check twice per a file.
No better than the original policy.
> Getting the next cluster The error handling already exists at the end of
> the while loop, so the code is duplicated.
> These costs should be paid next time and are no longer the "minimum cost".
I agree with your words, "These costs should be paid next time".
If so, how about moving the cluster handling for a hint dentry to
the beginning of the function while keeping the original policy?
BTW, this patch is not related to the hint code.
I think it would be better to keep the original code in this patch and improve
it with a separate patch.
> Should I add this to the commit-message?
>
>
> BR
> ---
> Tetsuhiro Kohada
RE: [PATCH v3] exfat: remove EXFAT_SB_DIRTY flag
> On 2020/06/18 22:11, Sungjong Seo wrote: > >> BTW > >> Even with this patch applied, VOL_DIRTY remains until synced in the > >> above case. > >> It's not easy to reproduce as rmdir, but I'll try to fix it in the > future. > > > > I think it's not a problem not to clear VOL_DIRTY under real errors, > > because VOL_DIRTY is just like a hint to note that write was not > finished clearly. > > > > If you mean there are more situation like ENOTEMPTY you mentioned, > > please make new patch to fix them. > > > When should VOL_DIRTY be cleared? > > The current behavior is ... > > Case of mkdir, rmdir, rename: >- set VOL_DIRTY before operation >- set VOL_CLEAN after operating. > In async mode, it is actually written to the media after 30 seconds. > > Case of cp, touch: >- set VOL_DIRTY before operation >- however, VOL_CLEAN is not called in this context. > VOL_CLEAN will call by sync_fs or unmount. > > I added VOL_CLEAN in last of __exfat_write_inode() and exfat_map_cluster(). > As a result, VOL_DIRTY is cleared with cp and touch. > However, when copying a many files ... > - Async mode: VOL_DIRTY is written to the media twice every 30 seconds. > - Sync mode: Of course, VOL_DIRTY and VOL_CLEAN to the media for each > file. > > Frequent writing VOL_DIRTY and VOL_CLEAN increases the risk of boot- > sector curruption. > If the boot-sector corrupted, it causes the following serious problems on > some OSs. > - misjudge as unformatted > - can't judge as exfat > - can't repair > > I want to minimize boot sector writes, to reduce these risk. > > I looked vfat/udf implementation, which manages similar dirty information > on linux, and found that they ware mark-dirty at mount and cleared at > unmount. > > Here are some ways to clear VOL_DIRTY. > > (A) VOL_CLEAN after every write operation. >:-) Ejectable at any time after a write operation. >:-( Many times write to Boot-sector. > > (B) dirty at mount, clear at unmount (same as vfat/udf) >:-) Write to boot-sector twice. >:-( It remains dirty unless unmounted. >:-( Write to boot-sector even if there is no write operation. > > (C) dirty on first write operation, clear on unmount >:-) Writing to boot-sector is minimal. >:-) Will not write to the boot-sector if there is no write operation. >:-( It remains dirty unless unmounted. > > (D) dirty on first write operation, clear on sync-fs/unmount > :-) Writing to boot-sector can be reduced. > :-) Will not write to the boot-sector if there is no write operation. > :-) sync-fs makes it clean and ejectable immidiately. > :-( It remains dirty unless sync-fs or unmount. > :-( Frequent sync-fs will increases writes to boot-sector. > > I think it should be (C) or(D). > What do you think? > First of all, I'm sorry for the late reply. And thank you for the suggestion. Most of the NAND flash devices and HDDs have wear leveling and bad sector replacement algorithms applied. So I think that the life of the boot sector will not be exhausted first. Currently the volume dirty/clean policy of exfat-fs is not perfect, but I think it behaves similarly to the policy of MS Windows. Therefore, I think code improvements should be made to reduce volume flag records while maintaining the current policy. BR Sungjong Seo > > > BR > --- > Tetsuhiro Kohada
RE: [PATCH 2/2] exfat: unify name extraction
> Name extraction in exfat_find_dir_entry() also doesn't care NameLength, so
> the name may be incorrect.
> Replace the name extraction in exfat_find_dir_entry() with using
> exfat_entry_set_cache and exfat_get_uniname_from_name_entries(),
> like exfat_readdir().
> Replace the name extraction with using exfat_entry_set_cache and
> exfat_get_uniname_from_name_entries(), like exfat_readdir().
> And, remove unused functions/parameters.
>
> ** This patch depends on:
> '[PATCH v3] exfat: integrates dir-entry getting and validation'.
>
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/dir.c | 161 ++--
> fs/exfat/exfat_fs.h | 2 +-
> fs/exfat/namei.c| 4 +-
> 3 files changed, 38 insertions(+), 129 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 545bb73b95e9..c9715c7a55a1 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -10,24 +10,6 @@
> #include "exfat_raw.h"
> #include "exfat_fs.h"
[snip]
> @@ -963,80 +942,38 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei,
> num_empty = 0;
> candi_empty.eidx = EXFAT_HINT_NONE;
>
[snip]
>
> - if (entry_type &
> - (TYPE_CRITICAL_SEC |
TYPE_BENIGN_SEC)) {
> - if (step == DIRENT_STEP_SECD) {
> - if (++order == num_ext)
> - goto found;
> - continue;
> - }
> + exfat_get_uniname_from_name_entries(es, _name);
It is needed to check a return value.
> + exfat_free_dentry_set(es, false);
> +
> + if (!exfat_uniname_ncmp(sb,
> + p_uniname->name,
> + uni_name.name,
> + name_len)) {
> + /* set the last used position as hint */
> + hint_stat->clu = clu.dir;
> + hint_stat->eidx = dentry;
eidx and clu of hint_stat should have one for the next entry we'll start
looking for.
Did you intentionally change the concept?
> + return dentry;
> }
> - step = DIRENT_STEP_FILE;
> }
>
> if (clu.flags == ALLOC_NO_FAT_CHAIN) { @@ -1069,32 +1006,6
> @@ int exfat_find_dir_entry(struct super_block *sb, struct
> exfat_inode_info *ei,
> hint_stat->clu = p_dir->dir;
> hint_stat->eidx = 0;
> return -ENOENT;
> -
> -found:
> - /* next dentry we'll find is out of this cluster */
> - if (!((dentry + 1) & (dentries_per_clu - 1))) {
> - int ret = 0;
> -
> - if (clu.flags == ALLOC_NO_FAT_CHAIN) {
> - if (--clu.size > 0)
> - clu.dir++;
> - else
> - clu.dir = EXFAT_EOF_CLUSTER;
> - } else {
> - ret = exfat_get_next_cluster(sb, );
> - }
> -
> - if (ret || clu.dir == EXFAT_EOF_CLUSTER) {
> - /* just initialized hint_stat */
> - hint_stat->clu = p_dir->dir;
> - hint_stat->eidx = 0;
> - return (dentry - num_ext);
> - }
> - }
> -
> - hint_stat->clu = clu.dir;
> - hint_stat->eidx = dentry + 1;
> - return dentry - num_ext;
> }
>
> int exfat_count_ext_entries(struct super_block *sb, struct exfat_chain
> *p_dir, diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> b88b7abc25bd..62a4768a4f6e 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -456,7 +456,7 @@ void exfat_update_dir_chksum_with_entry_set(struct
> exfat_entry_set_cache *es); int exfat_calc_num_entries(struct
> exfat_uni_name *p_uniname); int exfat_find_dir_entry(struct super_block
> *sb, struct exfat_inode_info *ei,
> struct exfat_chain *p_dir, struct exfat_uni_name *p_uniname,
> - int num_entries, unsigned int type);
> + int num_entries);
> int exfat_alloc_new_dir(struct inode *inode, struct exfat_chain *clu);
> int exfat_find_location(struct super_block *sb, struct exfat_chain *p_dir,
> int entry, sector_t *sector, int *offset); diff --git
> a/fs/exfat/namei.c b/fs/exfat/namei.c index a65d60ef93f4..c59d523547ca
> 100644
> --- a/fs/exfat/namei.c
> +++ b/fs/exfat/namei.c
> @@ -625,9 +625,7 @@ static int exfat_find(struct inode *dir, struct qstr
> *qname,
> }
>
> /* search the file name for directories */
> - dentry = exfat_find_dir_entry(sb, ei, , _name,
> - num_entries, TYPE_ALL);
> -
> + dentry = exfat_find_dir_entry(sb, ei, , _name,
> num_entries);
> if ((dentry < 0) &&
RE: [PATCH 1/2] exfat: add NameLength check when extracting name
> The current implementation doesn't care NameLength when extracting the
> name from Name dir-entries, so the name may be incorrect.
> (Without null-termination, Insufficient Name dir-entry, etc) Add a
> NameLength check when extracting the name from Name dir-entries to extract
> correct name.
> And, change to get the information of file/stream-ext dir-entries via the
> member variable of exfat_entry_set_cache.
>
> ** This patch depends on:
> '[PATCH v3] exfat: integrates dir-entry getting and validation'.
>
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/dir.c | 81 --
> 1 file changed, 39 insertions(+), 42 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 91cdbede0fd1..545bb73b95e9 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -28,16 +28,15 @@ static int exfat_extract_uni_name(struct exfat_dentry
> *ep,
>
> }
>
> -static void exfat_get_uniname_from_ext_entry(struct super_block *sb,
> - struct exfat_chain *p_dir, int entry, unsigned short
> *uniname)
> +static int exfat_get_uniname_from_name_entries(struct
> exfat_entry_set_cache *es,
> + struct exfat_uni_name *uniname)
> {
> - int i;
> - struct exfat_entry_set_cache *es;
> + int n, l, i;
> struct exfat_dentry *ep;
>
> - es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> - if (!es)
> - return;
> + uniname->name_len = es->de_stream->name_len;
> + if (uniname->name_len == 0)
> + return -EIO;
-EINVAL looks better.
>
> /*
>* First entry : file entry
> @@ -45,14 +44,15 @@ static void exfat_get_uniname_from_ext_entry(struct
> super_block *sb,
>* Third entry : first file-name entry
>* So, the index of first file-name dentry should start from 2.
>*/
> -
> - i = 2;
> - while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> - exfat_extract_uni_name(ep, uniname);
> - uniname += EXFAT_FILE_NAME_LEN;
> + for (l = 0, n = 2; l < uniname->name_len; n++) {
> + ep = exfat_get_validated_dentry(es, n, TYPE_NAME);
> + if (!ep)
> + return -EIO;
> + for (i = 0; l < uniname->name_len && i <
EXFAT_FILE_NAME_LEN;
> i++, l++)
> + uniname->name[l] = le16_to_cpu(ep-
> >dentry.name.unicode_0_14[i]);
Looks good.
> }
> -
> - exfat_free_dentry_set(es, false);
> + uniname->name[l] = 0;
> + return 0;
> }
>
> /* read a directory entry from the opened directory */ @@ -63,6 +63,7 @@
> static int exfat_readdir(struct inode *inode, struct exfat_dir_entry
> *dir_entry)
[snip]
> - *uni_name.name = 0x0;
> - exfat_get_uniname_from_ext_entry(sb, , dentry,
> - uni_name.name);
> + dir_entry->size = le64_to_cpu(es->de_stream-
> >valid_size);
> +
> + exfat_get_uniname_from_name_entries(es, _name);
Modified function has a return value.
It would be better to check the return value.
> exfat_utf16_to_nls(sb, _name,
> dir_entry->namebuf.lfn,
> dir_entry->namebuf.lfnbuf_len);
> - brelse(bh);
>
> - ep = exfat_get_dentry(sb, , i + 1, , NULL);
> - if (!ep)
> - return -EIO;
> - dir_entry->size =
> - le64_to_cpu(ep->dentry.stream.valid_size);
> - brelse(bh);
> + exfat_free_dentry_set(es, false);
>
> ei->hint_bmap.off = dentry >> dentries_per_clu_bits;
> ei->hint_bmap.clu = clu.dir;
> --
> 2.25.1
RE: [PATCH v3] exfat: integrates dir-entry getting and validation
> Add validation for num, bh and type on getting dir-entry. > Renamed exfat_get_dentry_cached() to exfat_get_validated_dentry() due to a > change in functionality. > > Integrate type-validation with simplified. > This will also recognize a dir-entry set that contains 'benign secondary' > dir-entries. > > Pre-Validated 'file' and 'stream-ext' dir-entries are provided as member > variables of exfat_entry_set_cache. > > And, rename TYPE_EXTEND to TYPE_NAME. > > Suggested-by: Sungjong Seo > Suggested-by: Namjae Jeon > Signed-off-by: Tetsuhiro Kohada Reviewed-by: Sungjong Seo Looks good to me. Thanks. > --- > Changes in v2 > - Change verification order > - Verification loop start with index 2 > Changes in v3 > - Fix indent > - Fix comment of exfat_get_dentry_set() > - Add de_file/de_stream in exfat_entry_set_cache > - Add srtuct tag name for each dir-entry type in exfat_dentry > - Add description about de_file/de_stream to commit-log > > fs/exfat/dir.c | 147 +-- > fs/exfat/exfat_fs.h | 17 +++-- > fs/exfat/exfat_raw.h | 10 +-- > fs/exfat/file.c | 25 > fs/exfat/inode.c | 49 ++- > fs/exfat/namei.c | 36 +-- > 6 files changed, 122 insertions(+), 162 deletions(-)
RE: [RFC]PATCH] exfat: integrates dir-entry getting and validation
> Add validation for num, bh and type on getting dir-entry.
> ('file' and 'stream-ext' dir-entries are pre-validated to ensure success)
> Renamed exfat_get_dentry_cached() to exfat_get_validated_dentry() due to a
> change in functionality.
>
> Integrate type-validation with simplified.
> This will also recognize a dir-entry set that contains 'benign secondary'
> dir-entries.
>
> And, rename TYPE_EXTEND to TYPE_NAME.
>
> Suggested-by: Sungjong Seo
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/dir.c | 144 ++--
> fs/exfat/exfat_fs.h | 15 +++--
> fs/exfat/file.c | 4 +-
> fs/exfat/inode.c| 6 +-
> fs/exfat/namei.c| 4 +-
> 5 files changed, 73 insertions(+), 100 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> f4cea9a7fd02..e029e0986edc 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
[snip]
> */
> struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block
*sb,
> - struct exfat_chain *p_dir, int entry, unsigned int type)
> + struct exfat_chain *p_dir, int entry, int max_entries)
> {
> int ret, i, num_bh;
> - unsigned int off, byte_offset, clu = 0;
> + unsigned int byte_offset, clu = 0;
> sector_t sec;
> struct exfat_sb_info *sbi = EXFAT_SB(sb);
> struct exfat_entry_set_cache *es;
> struct exfat_dentry *ep;
> - int num_entries;
> - enum exfat_validate_dentry_mode mode = ES_MODE_STARTED;
> struct buffer_head *bh;
>
> if (p_dir->dir == DIR_DELETED) {
> @@ -844,13 +815,13 @@ struct exfat_entry_set_cache
> *exfat_get_dentry_set(struct super_block *sb,
> return NULL;
> es->sb = sb;
> es->modified = false;
> + es->num_entries = 1;
>
> /* byte offset in cluster */
> byte_offset = EXFAT_CLU_OFFSET(byte_offset, sbi);
>
> /* byte offset in sector */
> - off = EXFAT_BLK_OFFSET(byte_offset, sb);
> - es->start_off = off;
> + es->start_off = EXFAT_BLK_OFFSET(byte_offset, sb);
>
> /* sector offset in cluster */
> sec = EXFAT_B_TO_BLK(byte_offset, sb); @@ -861,15 +832,12 @@ struct
> exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> goto free_es;
> es->bh[es->num_bh++] = bh;
>
> - ep = exfat_get_dentry_cached(es, 0);
> - if (!exfat_validate_entry(exfat_get_entry_type(ep), ))
> + ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> + if (!ep)
> goto free_es;
> + es->num_entries = min(ep->dentry.file.num_ext + 1, max_entries);
>
> - num_entries = type == ES_ALL_ENTRIES ?
> - ep->dentry.file.num_ext + 1 : type;
> - es->num_entries = num_entries;
> -
> - num_bh = EXFAT_B_TO_BLK_ROUND_UP(off + num_entries * DENTRY_SIZE,
> sb);
> + num_bh = EXFAT_B_TO_BLK_ROUND_UP(es->start_off + es->num_entries *
> +DENTRY_SIZE, sb);
> for (i = 1; i < num_bh; i++) {
> /* get the next sector */
> if (exfat_is_last_sector_in_cluster(sbi, sec)) { @@ -889,11
> +857,13 @@ struct exfat_entry_set_cache *exfat_get_dentry_set(struct
> super_block *sb,
> }
>
> /* validiate cached dentries */
> - for (i = 1; i < num_entries; i++) {
> - ep = exfat_get_dentry_cached(es, i);
> - if (!exfat_validate_entry(exfat_get_entry_type(ep), ))
> + for (i = 1; i < es->num_entries; i++) {
> + if (!exfat_get_validated_dentry(es, i, TYPE_SECONDARY))
> goto free_es;
> }
> + if (!exfat_get_validated_dentry(es, 1, TYPE_STREAM))
> + goto free_es;
It looks better to move checking TYPE_STREAM above the for-loop.
And then for-loop should start from index 2.
BTW, do you think it is enough to check only TYPE_SECONDARY not TYPE NAME?
As you might know, FILE, STREAM and NAME entries must be consecutive in
order.
> +
> return es;
>
RE: [PATCH v2] exfat: optimize exfat_zeroed_cluster()
> Replace part of exfat_zeroed_cluster() with exfat_update_bhs().
> And remove exfat_sync_bhs().
>
> Signed-off-by: Tetsuhiro Kohada
Reviewed-by: Sungjong Seo
Looks good. Thanks.
> ---
> Changes in v2
> - Rebase to latest exfat-dev
>
> fs/exfat/fatent.c | 53 +--
> 1 file changed, 10 insertions(+), 43 deletions(-)
>
> diff --git a/fs/exfat/fatent.c b/fs/exfat/fatent.c index
> 82ee8246c080..c3c9afee7418 100644
> --- a/fs/exfat/fatent.c
> +++ b/fs/exfat/fatent.c
> @@ -229,21 +229,6 @@ int exfat_find_last_cluster(struct super_block *sb,
> struct exfat_chain *p_chain,
> return 0;
> }
>
> -static inline int exfat_sync_bhs(struct buffer_head **bhs, int nr_bhs) -{
> - int i, err = 0;
> -
> - for (i = 0; i < nr_bhs; i++)
> - write_dirty_buffer(bhs[i], 0);
> -
> - for (i = 0; i < nr_bhs; i++) {
> - wait_on_buffer(bhs[i]);
> - if (!err && !buffer_uptodate(bhs[i]))
> - err = -EIO;
> - }
> - return err;
> -}
> -
> int exfat_zeroed_cluster(struct inode *dir, unsigned int clu) {
> struct super_block *sb = dir->i_sb;
> @@ -265,41 +250,23 @@ int exfat_zeroed_cluster(struct inode *dir, unsigned
> int clu)
> }
>
> /* Zeroing the unused blocks on this cluster */
> - n = 0;
> while (blknr < last_blknr) {
> - bhs[n] = sb_getblk(sb, blknr);
> - if (!bhs[n]) {
> - err = -ENOMEM;
> - goto release_bhs;
> - }
> - memset(bhs[n]->b_data, 0, sb->s_blocksize);
> - exfat_update_bh(bhs[n], 0);
> -
> - n++;
> - blknr++;
> -
> - if (n == nr_bhs) {
> - if (IS_DIRSYNC(dir)) {
> - err = exfat_sync_bhs(bhs, n);
> - if (err)
> - goto release_bhs;
> + for (n = 0; n < nr_bhs && blknr < last_blknr; n++, blknr++)
> {
> + bhs[n] = sb_getblk(sb, blknr);
> + if (!bhs[n]) {
> + err = -ENOMEM;
> + goto release_bhs;
> }
> -
> - for (i = 0; i < n; i++)
> - brelse(bhs[i]);
> - n = 0;
> + memset(bhs[n]->b_data, 0, sb->s_blocksize);
> }
> - }
>
> - if (IS_DIRSYNC(dir)) {
> - err = exfat_sync_bhs(bhs, n);
> + err = exfat_update_bhs(bhs, n, IS_DIRSYNC(dir));
> if (err)
> goto release_bhs;
> - }
> -
> - for (i = 0; i < n; i++)
> - brelse(bhs[i]);
>
> + for (i = 0; i < n; i++)
> + brelse(bhs[i]);
> + }
> return 0;
>
> release_bhs:
> --
> 2.25.1
RE: [PATCH v3] exfat: remove EXFAT_SB_DIRTY flag
> > Since this patch does not resolve 'VOL_DIRTY in ENOTEMPTY' problem you > > mentioned, it would be better to remove the description above for that > > and to make new patch. > > I mentioned rmdir as an example. > However, this problem is not only with rmdirs. > VOL_DIRTY remains when some functions abort with an error. > In original, VOL_DIRTY is not cleared even if performe 'sync'. > With this patch, it ensures that VOL_DIRTY will be cleared by 'sync'. > > Is my description insufficient? I understood what you said. However, it is a natural result when deleting the related code with EXFAT_SB_DIRTY flag. So I thought it would be better to separate it into new problems related to VOL_DIRTY-set under not real errors. > > > BTW > Even with this patch applied, VOL_DIRTY remains until synced in the above > case. > It's not easy to reproduce as rmdir, but I'll try to fix it in the future. I think it's not a problem not to clear VOL_DIRTY under real errors, because VOL_DIRTY is just like a hint to note that write was not finished clearly. If you mean there are more situation like ENOTEMPTY you mentioned, please make new patch to fix them. Thanks. > > > BR > --- > Tetsuhiro Kohada > >
[PATCH] exfat: flush dirty metadata in fsync
generic_file_fsync() exfat used could not guarantee the consistency of
a file because it has flushed not dirty metadata but only dirty data pages
for a file.
Instead of that, use exfat_file_fsync() for files and directories so that
it guarantees to commit both the metadata and data pages for a file.
Signed-off-by: Sungjong Seo
---
fs/exfat/dir.c | 2 +-
fs/exfat/exfat_fs.h | 1 +
fs/exfat/file.c | 19 ++-
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c
index 02acbb6ddf02..b71c540d88f2 100644
--- a/fs/exfat/dir.c
+++ b/fs/exfat/dir.c
@@ -309,7 +309,7 @@ const struct file_operations exfat_dir_operations = {
.llseek = generic_file_llseek,
.read = generic_read_dir,
.iterate= exfat_iterate,
- .fsync = generic_file_fsync,
+ .fsync = exfat_file_fsync,
};
int exfat_alloc_new_dir(struct inode *inode, struct exfat_chain *clu)
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 84664024e51e..6ec253581b86 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -417,6 +417,7 @@ void exfat_truncate(struct inode *inode, loff_t size);
int exfat_setattr(struct dentry *dentry, struct iattr *attr);
int exfat_getattr(const struct path *path, struct kstat *stat,
unsigned int request_mask, unsigned int query_flags);
+int exfat_file_fsync(struct file *file, loff_t start, loff_t end, int
datasync);
/* namei.c */
extern const struct dentry_operations exfat_dentry_ops;
diff --git a/fs/exfat/file.c b/fs/exfat/file.c
index fce03f318787..3b7fea465fd4 100644
--- a/fs/exfat/file.c
+++ b/fs/exfat/file.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include "exfat_raw.h"
#include "exfat_fs.h"
@@ -346,12 +347,28 @@ int exfat_setattr(struct dentry *dentry, struct iattr
*attr)
return error;
}
+int exfat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
+{
+ struct inode *inode = filp->f_mapping->host;
+ int err;
+
+ err = __generic_file_fsync(filp, start, end, datasync);
+ if (err)
+ return err;
+
+ err = sync_blockdev(inode->i_sb->s_bdev);
+ if (err)
+ return err;
+
+ return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL);
+}
+
const struct file_operations exfat_file_operations = {
.llseek = generic_file_llseek,
.read_iter = generic_file_read_iter,
.write_iter = generic_file_write_iter,
.mmap = generic_file_mmap,
- .fsync = generic_file_fsync,
+ .fsync = exfat_file_fsync,
.splice_read= generic_file_splice_read,
.splice_write = iter_file_splice_write,
};
--
2.17.1
RE: [PATCH v2] exfat: call sync_filesystem for read-only remount
> We need to commit dirty metadata and pages to disk before remounting exfat
> as read-only.
>
> This fixes a failure in xfstests generic/452
>
> generic/452 does the following:
> cp something /
> mount -o remount,ro
>
> the /something is corrupted. because while exfat is remounted as
> read-only, exfat doesn't have a chance to commit metadata and vfs
> invalidates page caches in a block device.
>
> Signed-off-by: Hyunchul Lee
Acked-by: Sungjong Seo
> ---
> Changes from v1:
> - Does not check the return value of sync_filesystem to
> allow to change from "rw" to "ro" even when this function
> fails.
> - Add the detailed explanation why generic/452 fails
>
> fs/exfat/super.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> e650e65536f8..253a92460d52 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -693,10 +693,20 @@ static void exfat_free(struct fs_context *fc)
> }
> }
>
> +static int exfat_reconfigure(struct fs_context *fc) {
> + fc->sb_flags |= SB_NODIRATIME;
> +
> + /* volume flag will be updated in exfat_sync_fs */
> + sync_filesystem(fc->root->d_sb);
> + return 0;
> +}
> +
> static const struct fs_context_operations exfat_context_ops = {
> .parse_param= exfat_parse_param,
> .get_tree = exfat_get_tree,
> .free = exfat_free,
> + .reconfigure= exfat_reconfigure,
> };
>
> static int exfat_init_fs_context(struct fs_context *fc)
> --
> 2.17.1
RE: [PATCH v3] exfat: remove EXFAT_SB_DIRTY flag
> remove EXFAT_SB_DIRTY flag and related codes.
>
> This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid
> sync_blockdev().
> However ...
> - exfat_put_super():
> Before calling this, the VFS has already called sync_filesystem(), so sync
> is never performed here.
> - exfat_sync_fs():
> After calling this, the VFS calls sync_blockdev(), so, it is meaningless
> to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here.
> Not only that, but in some cases can't clear VOL_DIRTY.
> ex:
> VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected,
> return error without setting EXFAT_SB_DIRTY.
> If performe 'sync' in this state, VOL_DIRTY will not be cleared.
>
Since this patch does not resolve 'VOL_DIRTY in ENOTEMPTY' problem you
mentioned,
it would be better to remove the description above for that and to make new
patch.
> Remove the EXFAT_SB_DIRTY check to ensure synchronization.
> And, remove the code related to the flag.
>
> Signed-off-by: Tetsuhiro Kohada
Reviewed-by: Sungjong Seo
> ---
> Changes in v2:
> - exfat_sync_fs() avoids synchronous processing when wait=0 Changes in
v3:
> - fix return value in exfat_sync_fs()
>
> fs/exfat/balloc.c | 4 ++--
> fs/exfat/dir.c | 16
> fs/exfat/exfat_fs.h | 5 +
> fs/exfat/fatent.c | 7 ++-
> fs/exfat/misc.c | 3 +--
> fs/exfat/namei.c| 12 ++--
> fs/exfat/super.c| 14 ++
> 7 files changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index
> 4055eb00ea9b..a987919686c0 100644
> --- a/fs/exfat/balloc.c
> +++ b/fs/exfat/balloc.c
> @@ -158,7 +158,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int
> clu)
> b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
>
> set_bit_le(b, sbi->vol_amap[i]->b_data);
> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
> return 0;
> }
>
> @@ -180,7 +180,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned
> int clu)
> b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
>
> clear_bit_le(b, sbi->vol_amap[i]->b_data);
> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
>
> if (opts->discard) {
> int ret_discard;
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 8e775bd5d523..02acbb6ddf02 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -470,7 +470,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> >dentry.file.access_date,
> NULL);
>
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
> brelse(bh);
>
> ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -
> 480,7 +480,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> exfat_init_stream_entry(ep,
> (type == TYPE_FILE) ? ALLOC_FAT_CHAIN : ALLOC_NO_FAT_CHAIN,
> start_clu, size);
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
> brelse(bh);
>
> return 0;
> @@ -516,7 +516,7 @@ int exfat_update_dir_chksum(struct inode *inode,
> struct exfat_chain *p_dir,
> }
>
> fep->dentry.file.checksum = cpu_to_le16(chksum);
> - exfat_update_bh(sb, fbh, IS_DIRSYNC(inode));
> + exfat_update_bh(fbh, IS_DIRSYNC(inode));
> release_fbh:
> brelse(fbh);
> return ret;
> @@ -538,7 +538,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> return -EIO;
>
> ep->dentry.file.num_ext = (unsigned char)(num_entries - 1);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
> brelse(bh);
>
> ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -
> 547,7 +547,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
> exfat_chain *p_dir,
>
> ep->dentry.stream.name_len = p_uniname->name_len;
> ep->dentry.stream.name_hash = cpu_to_le16(p_uniname->name_hash);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
> brelse(bh);
>
> for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) { @@ -556,7
> +556,7 @@ int exfat_init_ext_entry(struct inode *inode, struct exfat_chain
> *p_dir,
> return -EIO;
>
> exfat_init_name_entry(ep, uniname);
> - exfat_update_bh(sb, bh, sync);
> + e
RE: [PATCH v2] exfat: remove EXFAT_SB_DIRTY flag
> remove EXFAT_SB_DIRTY flag and related codes.
>
> This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid
> sync_blockdev().
> However ...
> - exfat_put_super():
> Before calling this, the VFS has already called sync_filesystem(), so sync
> is never performed here.
> - exfat_sync_fs():
> After calling this, the VFS calls sync_blockdev(), so, it is meaningless
> to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here.
> Not only that, but in some cases can't clear VOL_DIRTY.
> ex:
> VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected,
> return error without setting EXFAT_SB_DIRTY.
> If performe 'sync' in this state, VOL_DIRTY will not be cleared.
>
> Remove the EXFAT_SB_DIRTY check to ensure synchronization.
> And, remove the code related to the flag.
>
> Suggested-by: Sungjong Seo
> Signed-off-by: Tetsuhiro Kohada
> ---
> Changes in v2:
> - exfat_sync_fs() avoids synchronous processing when wait=0
>
> fs/exfat/balloc.c | 4 ++--
> fs/exfat/dir.c | 16
> fs/exfat/exfat_fs.h | 5 +
> fs/exfat/fatent.c | 7 ++-
> fs/exfat/misc.c | 3 +--
> fs/exfat/namei.c| 12 ++--
> fs/exfat/super.c| 14 ++
> 7 files changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/fs/exfat/balloc.c b/fs/exfat/balloc.c index
> 4055eb00ea9b..a987919686c0 100644
> --- a/fs/exfat/balloc.c
> +++ b/fs/exfat/balloc.c
> @@ -158,7 +158,7 @@ int exfat_set_bitmap(struct inode *inode, unsigned int
> clu)
> b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
>
> set_bit_le(b, sbi->vol_amap[i]->b_data);
> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
> return 0;
> }
>
> @@ -180,7 +180,7 @@ void exfat_clear_bitmap(struct inode *inode, unsigned
> int clu)
> b = BITMAP_OFFSET_BIT_IN_SECTOR(sb, ent_idx);
>
> clear_bit_le(b, sbi->vol_amap[i]->b_data);
> - exfat_update_bh(sb, sbi->vol_amap[i], IS_DIRSYNC(inode));
> + exfat_update_bh(sbi->vol_amap[i], IS_DIRSYNC(inode));
>
> if (opts->discard) {
> int ret_discard;
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 8e775bd5d523..02acbb6ddf02 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -470,7 +470,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> >dentry.file.access_date,
> NULL);
>
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
> brelse(bh);
>
> ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -
> 480,7 +480,7 @@ int exfat_init_dir_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> exfat_init_stream_entry(ep,
> (type == TYPE_FILE) ? ALLOC_FAT_CHAIN : ALLOC_NO_FAT_CHAIN,
> start_clu, size);
> - exfat_update_bh(sb, bh, IS_DIRSYNC(inode));
> + exfat_update_bh(bh, IS_DIRSYNC(inode));
> brelse(bh);
>
> return 0;
> @@ -516,7 +516,7 @@ int exfat_update_dir_chksum(struct inode *inode,
> struct exfat_chain *p_dir,
> }
>
> fep->dentry.file.checksum = cpu_to_le16(chksum);
> - exfat_update_bh(sb, fbh, IS_DIRSYNC(inode));
> + exfat_update_bh(fbh, IS_DIRSYNC(inode));
> release_fbh:
> brelse(fbh);
> return ret;
> @@ -538,7 +538,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
> exfat_chain *p_dir,
> return -EIO;
>
> ep->dentry.file.num_ext = (unsigned char)(num_entries - 1);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
> brelse(bh);
>
> ep = exfat_get_dentry(sb, p_dir, entry + 1, , ); @@ -
> 547,7 +547,7 @@ int exfat_init_ext_entry(struct inode *inode, struct
> exfat_chain *p_dir,
>
> ep->dentry.stream.name_len = p_uniname->name_len;
> ep->dentry.stream.name_hash = cpu_to_le16(p_uniname->name_hash);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
> brelse(bh);
>
> for (i = EXFAT_FIRST_CLUSTER; i < num_entries; i++) { @@ -556,7
> +556,7 @@ int exfat_init_ext_entry(struct inode *inode, struct exfat_chain
> *p_dir,
> return -EIO;
>
> exfat_init_name_entry(ep, uniname);
> - exfat_update_bh(sb, bh, sync);
> + exfat_update_bh(bh, sync);
> brelse(bh);
> uniname += EXFAT_FILE_NAME_LEN;
> }
> @@ -580,7 +580,7 @@ int exfat_remove_entries(struct inode *inode, struct
> exfat_chain *p_dir,
RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag
> On 2020/06/12 17:34, Sungjong Seo wrote:
> >> remove EXFAT_SB_DIRTY flag and related codes.
> >>
> >> This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid
> >> sync_blockdev().
> >> However ...
> >> - exfat_put_super():
> >> Before calling this, the VFS has already called sync_filesystem(), so
> >> sync is never performed here.
> >> - exfat_sync_fs():
> >> After calling this, the VFS calls sync_blockdev(), so, it is
> >> meaningless to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here.
> >> Not only that, but in some cases can't clear VOL_DIRTY.
> >> ex:
> >> VOL_DIRTY is set when rmdir starts, but when non-empty-dir is
> >> detected, return error without setting EXFAT_SB_DIRTY.
> >> If performe 'sync' in this state, VOL_DIRTY will not be cleared.
> >>
> >> Remove the EXFAT_SB_DIRTY check to ensure synchronization.
> >> And, remove the code related to the flag.
> >>
> >> Signed-off-by: Tetsuhiro Kohada
> >> ---
> >> fs/exfat/balloc.c | 4 ++--
> >> fs/exfat/dir.c | 16
> >> fs/exfat/exfat_fs.h | 5 +
> >> fs/exfat/fatent.c | 7 ++-
> >> fs/exfat/misc.c | 3 +--
> >> fs/exfat/namei.c| 12 ++--
> >> fs/exfat/super.c| 11 +++
> >> 7 files changed, 23 insertions(+), 35 deletions(-)
> >>
> > [snip]
> >>
> >> @@ -62,11 +59,9 @@ static int exfat_sync_fs(struct super_block *sb,
> >> int
> >> wait)
> >>
> >>/* If there are some dirty buffers in the bdev inode */
> >>mutex_lock(>s_lock);
> >> - if (test_and_clear_bit(EXFAT_SB_DIRTY, >s_state)) {
> >> - sync_blockdev(sb->s_bdev);
> >> - if (exfat_set_vol_flags(sb, VOL_CLEAN))
> >> - err = -EIO;
> >> - }
> >
> > I looked through most codes related to EXFAT_SB_DIRTY and VOL_DIRTY.
> > And your approach looks good because all of them seem to be protected
> > by s_lock.
> >
> > BTW, as you know, sync_filesystem() calls sync_fs() with 'nowait'
> > first, and then calls it again with 'wait' twice. No need to sync with
> lock twice.
> > If so, isn't it okay to do nothing when wait is 0?
>
> I also think ‘do nothing when wait is 0’ as you say, but I'm still not
> sure.
>
> Some other Filesystems do nothing with nowait and just return.
> However, a few Filesystems always perform sync.
>
> sync_blockdev() waits for completion, so it may be inappropriate to call
> with nowait. (But it was called in the original code)
>
> I'm still not sure, so I excluded it in this patch.
> Is it okay to include it?
>
Yes, I think so. sync_filesystem() will call __sync_blockdev() without 'wait'
first.
So, it's enough to call sync_blockdev() with s_lock just one time.
>
> >> + sync_blockdev(sb->s_bdev);
> >> + if (exfat_set_vol_flags(sb, VOL_CLEAN))
> >> + err = -EIO;
> >>mutex_unlock(>s_lock);
> >>return err;
> >> }
> >> --
> >> 2.25.1
> >
> >
>
> BR
> ---
> Tetsuhiro Kohada
RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag
> remove EXFAT_SB_DIRTY flag and related codes.
>
> This flag is set/reset in exfat_put_super()/exfat_sync_fs() to avoid
> sync_blockdev().
> However ...
> - exfat_put_super():
> Before calling this, the VFS has already called sync_filesystem(), so sync
> is never performed here.
> - exfat_sync_fs():
> After calling this, the VFS calls sync_blockdev(), so, it is meaningless
> to check EXFAT_SB_DIRTY or to bypass sync_blockdev() here.
> Not only that, but in some cases can't clear VOL_DIRTY.
> ex:
> VOL_DIRTY is set when rmdir starts, but when non-empty-dir is detected,
> return error without setting EXFAT_SB_DIRTY.
> If performe 'sync' in this state, VOL_DIRTY will not be cleared.
>
> Remove the EXFAT_SB_DIRTY check to ensure synchronization.
> And, remove the code related to the flag.
>
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/balloc.c | 4 ++--
> fs/exfat/dir.c | 16
> fs/exfat/exfat_fs.h | 5 +
> fs/exfat/fatent.c | 7 ++-
> fs/exfat/misc.c | 3 +--
> fs/exfat/namei.c| 12 ++--
> fs/exfat/super.c| 11 +++
> 7 files changed, 23 insertions(+), 35 deletions(-)
>
[snip]
>
> @@ -62,11 +59,9 @@ static int exfat_sync_fs(struct super_block *sb, int
> wait)
>
> /* If there are some dirty buffers in the bdev inode */
> mutex_lock(>s_lock);
> - if (test_and_clear_bit(EXFAT_SB_DIRTY, >s_state)) {
> - sync_blockdev(sb->s_bdev);
> - if (exfat_set_vol_flags(sb, VOL_CLEAN))
> - err = -EIO;
> - }
I looked through most codes related to EXFAT_SB_DIRTY and VOL_DIRTY.
And your approach looks good because all of them seem to be protected by
s_lock.
BTW, as you know, sync_filesystem() calls sync_fs() with 'nowait' first,
and then calls it again with 'wait' twice. No need to sync with lock twice.
If so, isn't it okay to do nothing when wait is 0?
> + sync_blockdev(sb->s_bdev);
> + if (exfat_set_vol_flags(sb, VOL_CLEAN))
> + err = -EIO;
> mutex_unlock(>s_lock);
> return err;
> }
> --
> 2.25.1
RE: [PATCH 3/4 v4] exfat: add boot region verification
> Add Boot-Regions verification specified in exFAT specification.
> Note that the checksum type is strongly related to the raw structure, so
> the'u32 'type is used to clarify the number of bits.
>
> Signed-off-by: Tetsuhiro Kohada
Reviewed-by: Sungjong Seo
> ---
> Changes in v2:
> - rebase with patch 'optimize dir-cache' applied
> - just print a warning when invalid exboot-signature detected
> - print additional information when invalid boot-checksum detected
> Changes in v3:
> - based on '[PATCH 2/4 v3] exfat: separate the boot sector analysis'
> Changes in v4:
> - fix type of p_sig/p_chksum to __le32
>
> fs/exfat/exfat_fs.h | 1 +
> fs/exfat/misc.c | 14 +
> fs/exfat/super.c| 50 +
> 3 files changed, 65 insertions(+)
>
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> 9673e2d31045..eebbe5a84b2b 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -514,6 +514,7 @@ void exfat_set_entry_time(struct exfat_sb_info *sbi,
> struct timespec64 *ts,
> u8 *tz, __le16 *time, __le16 *date, u8 *time_cs); unsigned
> short exfat_calc_chksum_2byte(void *data, int len,
> unsigned short chksum, int type);
> +u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type);
> void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int
> sync); void exfat_chain_set(struct exfat_chain *ec, unsigned int dir,
> unsigned int size, unsigned char flags); diff --git
> a/fs/exfat/misc.c b/fs/exfat/misc.c index ab7f88b1f6d3..b82d2dd5bd7c
> 100644
> --- a/fs/exfat/misc.c
> +++ b/fs/exfat/misc.c
> @@ -151,6 +151,20 @@ unsigned short exfat_calc_chksum_2byte(void *data,
> int len,
> return chksum;
> }
>
> +u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type) {
> + int i;
> + u8 *c = (u8 *)data;
> +
> + for (i = 0; i < len; i++, c++) {
> + if (unlikely(type == CS_BOOT_SECTOR &&
> + (i == 106 || i == 107 || i == 112)))
> + continue;
> + chksum = ((chksum << 31) | (chksum >> 1)) + *c;
> + }
> + return chksum;
> +}
> +
> void exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int
> sync) {
> set_bit(EXFAT_SB_DIRTY, _SB(sb)->s_state); diff --git
> a/fs/exfat/super.c b/fs/exfat/super.c index 6a1330be5a9a..405717e4e3ea
> 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -491,6 +491,50 @@ static int exfat_read_boot_sector(struct super_block
> *sb)
> return 0;
> }
>
> +static int exfat_verify_boot_region(struct super_block *sb) {
> + struct buffer_head *bh = NULL;
> + u32 chksum = 0;
> + __le32 *p_sig, *p_chksum;
> + int sn, i;
> +
> + /* read boot sector sub-regions */
> + for (sn = 0; sn < 11; sn++) {
> + bh = sb_bread(sb, sn);
> + if (!bh)
> + return -EIO;
> +
> + if (sn != 0 && sn <= 8) {
> + /* extended boot sector sub-regions */
> + p_sig = (__le32 *)>b_data[sb->s_blocksize - 4];
> + if (le32_to_cpu(*p_sig) != EXBOOT_SIGNATURE)
> + exfat_warn(sb, "Invalid
exboot-signature(sector
> = %d): 0x%08x",
> +sn, le32_to_cpu(*p_sig));
> + }
> +
> + chksum = exfat_calc_chksum32(bh->b_data, sb->s_blocksize,
> + chksum, sn ? CS_DEFAULT : CS_BOOT_SECTOR);
> + brelse(bh);
> + }
> +
> + /* boot checksum sub-regions */
> + bh = sb_bread(sb, sn);
> + if (!bh)
> + return -EIO;
> +
> + for (i = 0; i < sb->s_blocksize; i += sizeof(u32)) {
> + p_chksum = (__le32 *)>b_data[i];
> + if (le32_to_cpu(*p_chksum) != chksum) {
> + exfat_err(sb, "Invalid boot checksum (boot checksum
:
> 0x%08x, checksum : 0x%08x)",
> + le32_to_cpu(*p_chksum), chksum);
> + brelse(bh);
> + return -EINVAL;
> + }
> + }
> + brelse(bh);
> + return 0;
> +}
> +
> /* mount the file system volume */
> static int __exfat_fill_super(struct super_block *sb) { @@ -503,6
> +547,12 @@ static int __exfat_fill_super(struct super_block *sb)
> goto free_bh;
> }
>
> + ret = exfat_verify_boot_region(sb);
> + if (ret) {
> + exfat_err(sb, "invalid boot region");
> + goto free_bh;
> + }
> +
> ret = exfat_create_upcase_table(sb);
> if (ret) {
> exfat_err(sb, "failed to load upcase table");
> --
> 2.25.1
RE: [PATCH 4/4 v3] exfat: standardize checksum calculation
> To clarify that it is a 16-bit checksum, the parts related to the 16-bit
> checksum are renamed and change type to u16.
> Furthermore, replace checksum calculation in exfat_load_upcase_table()
> with exfat_calc_checksum32().
>
> Signed-off-by: Tetsuhiro Kohada
Reviewed-by: Sungjong Seo
> ---
> Changes in v2:
> - rebase with patch 'optimize dir-cache' applied Changes in v3:
> - based on '[PATCH 2/4 v3] exfat: separate the boot sector analysis'
>
> fs/exfat/dir.c | 12 ++--
> fs/exfat/exfat_fs.h | 5 ++---
> fs/exfat/misc.c | 10 --
> fs/exfat/nls.c | 19 +++
> 4 files changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> 2902d285bf20..de43534aa299 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -491,7 +491,7 @@ int exfat_update_dir_chksum(struct inode *inode,
> struct exfat_chain *p_dir,
> int ret = 0;
> int i, num_entries;
> sector_t sector;
> - unsigned short chksum;
> + u16 chksum;
> struct exfat_dentry *ep, *fep;
> struct buffer_head *fbh, *bh;
>
> @@ -500,7 +500,7 @@ int exfat_update_dir_chksum(struct inode *inode,
> struct exfat_chain *p_dir,
> return -EIO;
>
> num_entries = fep->dentry.file.num_ext + 1;
> - chksum = exfat_calc_chksum_2byte(fep, DENTRY_SIZE, 0, CS_DIR_ENTRY);
> + chksum = exfat_calc_chksum16(fep, DENTRY_SIZE, 0, CS_DIR_ENTRY);
>
> for (i = 1; i < num_entries; i++) {
> ep = exfat_get_dentry(sb, p_dir, entry + i, , NULL); @@ -
> 508,7 +508,7 @@ int exfat_update_dir_chksum(struct inode *inode, struct
> exfat_chain *p_dir,
> ret = -EIO;
> goto release_fbh;
> }
> - chksum = exfat_calc_chksum_2byte(ep, DENTRY_SIZE, chksum,
> + chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
> CS_DEFAULT);
> brelse(bh);
> }
> @@ -593,8 +593,8 @@ void exfat_update_dir_chksum_with_entry_set(struct
> exfat_entry_set_cache *es)
>
> for (i = 0; i < es->num_entries; i++) {
> ep = exfat_get_dentry_cached(es, i);
> - chksum = exfat_calc_chksum_2byte(ep, DENTRY_SIZE, chksum,
> - chksum_type);
> + chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
> + chksum_type);
> chksum_type = CS_DEFAULT;
> }
> ep = exfat_get_dentry_cached(es, 0);
> @@ -1000,7 +1000,7 @@ int exfat_find_dir_entry(struct super_block *sb,
> struct exfat_inode_info *ei,
> }
>
> if (entry_type == TYPE_STREAM) {
> - unsigned short name_hash;
> + u16 name_hash;
>
> if (step != DIRENT_STEP_STRM) {
> step = DIRENT_STEP_FILE;
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> eebbe5a84b2b..9188985694f0 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -137,7 +137,7 @@ struct exfat_dentry_namebuf { struct exfat_uni_name {
> /* +3 for null and for converting */
> unsigned short name[MAX_NAME_LENGTH + 3];
> - unsigned short name_hash;
> + u16 name_hash;
> unsigned char name_len;
> };
>
> @@ -512,8 +512,7 @@ void exfat_get_entry_time(struct exfat_sb_info *sbi,
> struct timespec64 *ts, void exfat_truncate_atime(struct timespec64 *ts);
> void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64
*ts,
> u8 *tz, __le16 *time, __le16 *date, u8 *time_cs); -unsigned
> short exfat_calc_chksum_2byte(void *data, int len,
> - unsigned short chksum, int type);
> +u16 exfat_calc_chksum16(void *data, int len, u16 chksum, int type);
> u32 exfat_calc_chksum32(void *data, int len, u32 chksum, int type); void
> exfat_update_bh(struct super_block *sb, struct buffer_head *bh, int sync);
> void exfat_chain_set(struct exfat_chain *ec, unsigned int dir, diff --git
> a/fs/exfat/misc.c b/fs/exfat/misc.c index b82d2dd5bd7c..17d41f3d3709
> 100644
> --- a/fs/exfat/misc.c
> +++ b/fs/exfat/misc.c
> @@ -136,17 +136,15 @@ void exfat_truncate_atime(struct timespec64 *ts)
> ts->tv_nsec = 0;
> }
>
> -unsigned short exfat_calc_chksum_2byte(void *data, int len,
> - unsigned short chksum, int type)
> +u16 exfat_calc_chksum16(void *data, int len, u16 chksum, int type)
> {
> int i;
> - unsigned char *c = (unsigned char *)data;
> + u8 *c = (u8 *)da
RE: [PATCH 2/4 v3] exfat: separate the boot sector analysis
> Separate the boot sector analysis to read_boot_sector().
> And add a check for the fs_name field.
> Furthermore, add a strict consistency check, because overlapping areas can
> cause serious corruption.
>
> Signed-off-by: Tetsuhiro Kohada
Reviewed-by: Sungjong Seo
> ---
> Changes in v2:
> - rebase with patch 'optimize dir-cache' applied Changes in v3:
> - add a check for the fs_name field
>
> fs/exfat/exfat_raw.h | 2 +
> fs/exfat/super.c | 97
> 2 files changed, 56 insertions(+), 43 deletions(-)
>
> diff --git a/fs/exfat/exfat_raw.h b/fs/exfat/exfat_raw.h index
> 07f74190df44..350ce59cc324 100644
> --- a/fs/exfat/exfat_raw.h
> +++ b/fs/exfat/exfat_raw.h
> @@ -10,11 +10,13 @@
>
> #define BOOT_SIGNATURE 0xAA55
> #define EXBOOT_SIGNATURE 0xAA55
> +#define STR_EXFAT"EXFAT " /* size should be 8 */
>
> #define EXFAT_MAX_FILE_LEN 255
>
> #define VOL_CLEAN0x
> #define VOL_DIRTY0x0002
> +#define ERR_MEDIUM 0x0004
>
> #define EXFAT_EOF_CLUSTER0xu
> #define EXFAT_BAD_CLUSTER0xFFF7u
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> e60d28e73ff0..6a1330be5a9a 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -366,25 +366,20 @@ static int exfat_read_root(struct inode *inode)
> return 0;
> }
>
> -static struct boot_sector *exfat_read_boot_with_logical_sector(
> - struct super_block *sb)
> +static int exfat_calibrate_blocksize(struct super_block *sb, int
> +logical_sect)
> {
> struct exfat_sb_info *sbi = EXFAT_SB(sb);
> - struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh-
> >b_data;
> - unsigned short logical_sect = 0;
> -
> - logical_sect = 1 << p_boot->sect_size_bits;
>
> if (!is_power_of_2(logical_sect) ||
> logical_sect < 512 || logical_sect > 4096) {
> exfat_err(sb, "bogus logical sector size %u", logical_sect);
> - return NULL;
> + return -EIO;
> }
>
> if (logical_sect < sb->s_blocksize) {
> exfat_err(sb, "logical sector size too small for device
> (logical sector size = %u)",
> logical_sect);
> - return NULL;
> + return -EIO;
> }
>
> if (logical_sect > sb->s_blocksize) {
> @@ -394,24 +389,20 @@ static struct boot_sector
> *exfat_read_boot_with_logical_sector(
> if (!sb_set_blocksize(sb, logical_sect)) {
> exfat_err(sb, "unable to set blocksize %u",
> logical_sect);
> - return NULL;
> + return -EIO;
> }
> sbi->boot_bh = sb_bread(sb, 0);
> if (!sbi->boot_bh) {
> exfat_err(sb, "unable to read boot sector (logical
> sector size = %lu)",
> sb->s_blocksize);
> - return NULL;
> + return -EIO;
> }
> -
> - p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
> }
> - return p_boot;
> + return 0;
> }
>
> -/* mount the file system volume */
> -static int __exfat_fill_super(struct super_block *sb)
> +static int exfat_read_boot_sector(struct super_block *sb)
> {
> - int ret;
> struct boot_sector *p_boot;
> struct exfat_sb_info *sbi = EXFAT_SB(sb);
>
> @@ -424,51 +415,41 @@ static int __exfat_fill_super(struct super_block
*sb)
> exfat_err(sb, "unable to read boot sector");
> return -EIO;
> }
> -
> - /* PRB is read */
> p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
>
> /* check the validity of BOOT */
> if (le16_to_cpu((p_boot->signature)) != BOOT_SIGNATURE) {
> exfat_err(sb, "invalid boot record signature");
> - ret = -EINVAL;
> - goto free_bh;
> + return -EINVAL;
> }
>
> -
> - /* check logical sector size */
> - p_boot = exfat_read_boot_with_logical_sector(sb);
> - if (!p_boot) {
> - ret = -EIO;
> - goto free_bh;
> + if (memcmp(p_boot->fs_name, STR_EXFAT, BOOTSEC_FS_NAME_LEN)) {
> + exfat_err(sb, "invalid fs_name"); /* fs_name may unprintable
> */
> + return -EINVAL;
> }
>
> /*
> - * res_zero field must be filled with z
RE: [PATCH 1/4 v3] exfat: redefine PBR as boot_sector
> Aggregate PBR related definitions and redefine as "boot_sector" to comply
> with the exFAT specification.
> And, rename variable names including 'pbr'.
>
> Signed-off-by: Tetsuhiro Kohada
Reviewed-by: Sungjong Seo
> ---
> Changes in v2:
> - rebase with patch 'optimize dir-cache' applied Changes in v3:
> - rename BOOTSEC_OEM_NAME_LEN to BOOTSEC_FS_NAME_LEN
> - rename oem_name to fs_name in struct boot_sector
>
> fs/exfat/exfat_fs.h | 2 +-
> fs/exfat/exfat_raw.h | 79 +++--
> fs/exfat/super.c | 84 ++--
> 3 files changed, 72 insertions(+), 93 deletions(-)
>
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index
> 5caad1380818..9673e2d31045 100644
> --- a/fs/exfat/exfat_fs.h
> +++ b/fs/exfat/exfat_fs.h
> @@ -227,7 +227,7 @@ struct exfat_sb_info {
> unsigned int root_dir; /* root dir cluster */
> unsigned int dentries_per_clu; /* num of dentries per cluster */
> unsigned int vol_flag; /* volume dirty flag */
> - struct buffer_head *pbr_bh; /* buffer_head of PBR sector */
> + struct buffer_head *boot_bh; /* buffer_head of BOOT sector */
>
> unsigned int map_clu; /* allocation bitmap start cluster */
> unsigned int map_sectors; /* num of allocation bitmap sectors */
> diff --git a/fs/exfat/exfat_raw.h b/fs/exfat/exfat_raw.h index
> 8d6c64a7546d..07f74190df44 100644
> --- a/fs/exfat/exfat_raw.h
> +++ b/fs/exfat/exfat_raw.h
> @@ -8,7 +8,8 @@
>
> #include
>
> -#define PBR_SIGNATURE0xAA55
> +#define BOOT_SIGNATURE 0xAA55
> +#define EXBOOT_SIGNATURE 0xAA55
>
> #define EXFAT_MAX_FILE_LEN 255
>
> @@ -55,7 +56,7 @@
>
> /* checksum types */
> #define CS_DIR_ENTRY 0
> -#define CS_PBR_SECTOR1
> +#define CS_BOOT_SECTOR 1
> #define CS_DEFAULT 2
>
> /* file attributes */
> @@ -69,57 +70,35 @@
> #define ATTR_RWMASK (ATTR_HIDDEN | ATTR_SYSTEM | ATTR_VOLUME
> | \
>ATTR_SUBDIR | ATTR_ARCHIVE)
>
> -#define PBR64_JUMP_BOOT_LEN 3
> -#define PBR64_OEM_NAME_LEN 8
> -#define PBR64_RESERVED_LEN 53
> +#define BOOTSEC_JUMP_BOOT_LEN3
> +#define BOOTSEC_FS_NAME_LEN 8
> +#define BOOTSEC_OLDBPB_LEN 53
>
> #define EXFAT_FILE_NAME_LEN 15
>
> -/* EXFAT BIOS parameter block (64 bytes) */ -struct bpb64 {
> - __u8 jmp_boot[PBR64_JUMP_BOOT_LEN];
> - __u8 oem_name[PBR64_OEM_NAME_LEN];
> - __u8 res_zero[PBR64_RESERVED_LEN];
> -} __packed;
> -
> -/* EXFAT EXTEND BIOS parameter block (56 bytes) */ -struct bsx64 {
> - __le64 vol_offset;
> - __le64 vol_length;
> - __le32 fat_offset;
> - __le32 fat_length;
> - __le32 clu_offset;
> - __le32 clu_count;
> - __le32 root_cluster;
> - __le32 vol_serial;
> - __u8 fs_version[2];
> - __le16 vol_flags;
> - __u8 sect_size_bits;
> - __u8 sect_per_clus_bits;
> - __u8 num_fats;
> - __u8 phy_drv_no;
> - __u8 perc_in_use;
> - __u8 reserved2[7];
> -} __packed;
> -
> -/* EXFAT PBR[BPB+BSX] (120 bytes) */
> -struct pbr64 {
> - struct bpb64 bpb;
> - struct bsx64 bsx;
> -} __packed;
> -
> -/* Common PBR[Partition Boot Record] (512 bytes) */ -struct pbr {
> - union {
> - __u8 raw[64];
> - struct bpb64 f64;
> - } bpb;
> - union {
> - __u8 raw[56];
> - struct bsx64 f64;
> - } bsx;
> - __u8 boot_code[390];
> - __le16 signature;
> +/* EXFAT: Main and Backup Boot Sector (512 bytes) */ struct boot_sector
> +{
> + __u8jmp_boot[BOOTSEC_JUMP_BOOT_LEN];
> + __u8fs_name[BOOTSEC_FS_NAME_LEN];
> + __u8must_be_zero[BOOTSEC_OLDBPB_LEN];
> + __le64 partition_offset;
> + __le64 vol_length;
> + __le32 fat_offset;
> + __le32 fat_length;
> + __le32 clu_offset;
> + __le32 clu_count;
> + __le32 root_cluster;
> + __le32 vol_serial;
> + __u8fs_revision[2];
> + __le16 vol_flags;
> + __u8sect_size_bits;
> + __u8sect_per_clus_bits;
> + __u8num_fats;
> + __u8drv_sel;
> + __u8percent_in_use;
> + __u8reserved[7];
> + __u8boot_code[390];
> + __le16 signature;
> } __packed;
>
> struct exfat_dentry {
> diff --git a/fs/exfat/super.c b/fs/exfat/super.c index
> c1f47f4071a8..e60d28e73ff0 100644
> --- a/fs/exfat/super.c
> +++ b/fs/exfat/super.c
> @@ -49,7 +49,7 @@ static void exfat_put_super(st
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
>
Reviewed-by: Sungjong Seo
> ---
> 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(-)
>
> diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> b5a237c33d50..2902d285bf20 100644
> --- a/fs/exfat/dir.c
> +++ b/fs/exfat/dir.c
> @@ -32,35 +32,30 @@ static void exfat_get_uniname_from_ext_entry(struct
> super_block *sb,
> struct exfat_chain *p_dir, int entry, unsigned short
> *uniname) {
> int i;
> - struct exfat_dentry *ep;
> struct exfat_entry_set_cache *es;
>
> - es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES, );
> + es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> if (!es)
> return;
>
> - if (es->num_entries < 3)
> - goto free_es;
> -
> - ep += 2;
> -
> /*
>* First entry : file entry
>* Second entry : stream-extension entry
>* Third entry : first file-name entry
>* So, the index of first file-name dentry should start from 2.
>*/
> - for (i = 2; i < es->num_entries; i++, ep++) {
> + for (i = 2; i < es->num_entries; i++) {
> + struct exfat_dentry *ep = exfat_get_dentry_cached(es, i);
> +
> /* end of name entry */
> if (exfat_get_entry_type(ep) != TYPE_EXTEND)
> - goto free_es;
> + break;
>
> exfat_extract_uni_name(ep, uniname);
> uniname += EXFAT_FILE_NAME_LEN;
> }
>
> -free_es:
> - kfree(es);
> + exfat_free_dentry_set(es, false);
> }
>
> /* read a directory entry from the opened directory */ @@ -590,62 +585,33
> @@ int exfat_remove_entries(struct inode *inode, struct exfat_chain
*p_dir,
> return 0;
> }
>
> -int exfat_update_dir_chksum_with_entry_set(struct super_block *sb,
> - struct exfat_entry_set_cache *es, int sync)
> +void exfat_update_dir_chksum_with_entry_set(struct
> +exfat_entry_set_cache *es)
> {
> - struct exfat_sb_info *sbi = EXFAT_SB(sb);
> - struct buffer_head *bh;
> - sector_t sec = es->sector;
> - unsigned int off = es->offset;
> - int chksum_type = CS_DIR_ENTRY, i, num_entries = es->num_entries;
> - unsigned int buf_off = (off - es->offset);
> - unsigned int remaining_byte_in_sector, copy_entries, clu;
> + int chksum_type = CS_DIR_ENTRY, i;
> unsigned short chksum = 0;
> + struct exfat_dentry *ep;
>
> - for (i = 0; i < num_entries; i++) {
> - chksum = exfat_calc_chksum_2byte(>entries[i],
> DENTRY_SIZE,
> - chksum, chksum_type);
> + for (i = 0; i < es->num_entries; i++) {
> + ep = exfat_get_dentry_cached(es, i);
> + chksum = exfat_calc_chksum_2byte(ep, DENTRY_SIZE, chksum,
> + chksum_type);
> chksum_type = CS_DEFAULT;
> }
> + ep = exfat_get_dentry_cached(es, 0);
> + ep->dentry.file.checksum = cpu_to_le16(chksum);
> + es->modified = true;
> +}
>
> - 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 *)>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_clust
RE: [PATCH 1/4] exfat: redefine PBR as boot_sector
> > [snip]
> >> +/* EXFAT: Main and Backup Boot Sector (512 bytes) */ struct
> >> +boot_sector {
> >> + __u8jmp_boot[BOOTSEC_JUMP_BOOT_LEN];
> >> + __u8oem_name[BOOTSEC_OEM_NAME_LEN];
> >
> > According to the exFAT specification, fs_name and BOOTSEC_FS_NAME_LEN
> > look better.
>
> Oops.
> I sent v2 patches, before I noticed this comment,
>
> I'll make another small patch, OK?
No, It make sense to make v3, because you have renamed the variables in
boot_sector on this patch.
> BTW
> I have a concern about fs_name.
> The exfat specification says that this field is "EXFAT".
>
> I think it's a important field for determining the filesystem.
> However, in this patch, I gave up checking this field.
> Because there is no similar check in FATFS.
> Do you know why Linux FATFS does not check this filed?
> And, what do you think of checking this field?
FATFS has the same field named "oem_name" and whatever is okay for its value.
However, in case of exFAT, it is an important field to determine filesystem.
I think it would be better to check this field for exFAT-fs.
Would you like to contribute new patch for checking it?
>
> BR
RE: [PATCH 1/4] exfat: redefine PBR as boot_sector
> Aggregate PBR related definitions and redefine as "boot_sector" to comply
> with the exFAT specification.
> And, rename variable names including 'pbr'.
>
> Signed-off-by: Tetsuhiro Kohada
> ---
> fs/exfat/exfat_fs.h | 2 +-
> fs/exfat/exfat_raw.h | 79 +++--
> fs/exfat/super.c | 84 ++--
> 3 files changed, 72 insertions(+), 93 deletions(-)
>
[snip]
> +/* EXFAT: Main and Backup Boot Sector (512 bytes) */ struct boot_sector
> +{
> + __u8jmp_boot[BOOTSEC_JUMP_BOOT_LEN];
> + __u8oem_name[BOOTSEC_OEM_NAME_LEN];
According to the exFAT specification, fs_name and BOOTSEC_FS_NAME_LEN look
better.
> + __u8must_be_zero[BOOTSEC_OLDBPB_LEN];
> + __le64 partition_offset;
> + __le64 vol_length;
> + __le32 fat_offset;
> + __le32 fat_length;
> + __le32 clu_offset;
> + __le32 clu_count;
> + __le32 root_cluster;
> + __le32 vol_serial;
> + __u8fs_revision[2];
> + __le16 vol_flags;
> + __u8sect_size_bits;
> + __u8sect_per_clus_bits;
> + __u8num_fats;
> + __u8drv_sel;
> + __u8percent_in_use;
> + __u8reserved[7];
> + __u8boot_code[390];
> + __le16 signature;
> } __packed;
RE: [PATCH] exfat: optimize dir-cache
> 2020-05-27 17:00 GMT+09:00,
> kohada.tetsuh...@dc.mitsubishielectric.co.jp
> :
> > Thank you for your comment.
> >
> > >> +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.
> >
> > I thought the same, while creating this patch.
> > However this patch has changed a lot and I didn't add any new error
> > checking.
> > (So, the same behavior will occur even if an error occurs)
> >
> > >> +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.
> >
> > There is no new error checking for same reason as above.
> >
> > I'll try to add error checking to this v2 patch.
> > Or is it better to add error checking in another patch?
> The latter:)
> Thanks!
Yes, the latter looks better.
Thanks!
> >
> > BR
> > ---
> > Kohada Tetsuhiro
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
>
> ---
> 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 *)>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, ))
> - 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, );
> + 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, ,
> @@ -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);
> +
