Fw: RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()

2020-10-14 Thread kohada.tetsuh...@dc.mitsubishielectric.co.jp
Thank you for continuing the discussion.
The reply was delayed to summarize the arguing points.

> I already gave my comment on previous thread, and I prefer de array handling 
> I sent instead of only two entries.
We haven't discussed enough yet and I have some questions.
I still don't understand what's technically problem.

> It will avoid repetitive loops to get entries from cache buffer.
Is that loop the first verification and name extraction?
I don't understand why you can avoid the repetitive loop by using arrays.
I think getting from an array is equivalent to getting it via a function.
   A = obj->array[idx];
   B = get(obj, idx);
For using, what's the difference between A and B?

> I think it is also suitable for function definition and union type  entry 
> structure.
I, too, think the combination is not bad.
However, it has no advantage compared to other methods.
(Can you give me any example?)
Also, as I said in my previous mail, union has the problem of too flexible for 
type.
(Especially file-de and stream-de are easy to confuse)
So I want to avoid to access union directly from the upper function, as 
possible.

> If you send the patches included this change again, I will actively look into 
> your patches.
It will take some time as I haven't come up with a good idea yet.

We have discussed it so far, but there are still some unclear points.
First, I would like to clarify them.

1. About the need for TYPE_NAME-validation in exfat_get_dentry_set().
My opinion is
> It is possible to correctly determine that
> "Immediately follow the Stream Extension directory entry as a consecutive 
> series"
> whether the TYPE_NAME check is implemented exfat_get_uniname_from_ext_entry() 
> or
> exfat_get_dentry_set().
> It's functionally same, so it is also right to validate in either.

Your opinion is
> We have not checked the problem when it is removed because it was implemented
> according to the specification from the beginning.

I understand that you haven't thought about it yet.
What happens if I don't check here?
Please imagine if you can.


2. About TYPE_NAME-validation in exfat_get_uniname_from_ext_entry()
Below are the changes in '[PATCH v4 1/5] exfat: integrates dir-entry getting 
and validation'
> - 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)
> - break;
>
> + i = ES_INDEX_NAME;
> + while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
>   exfat_extract_uni_name(ep, uniname);
>   uniname += EXFAT_FILE_NAME_LEN;
>   }

Your request for this change is
> Please find the way to access name entries like ep_file, ep_stream
> without calling exfat_get_validated_dentry().

What is the reason(or rationale) for such a request?
Please explain what the problem is with this change, if you can.

As I explained before, the reason for validating TYPE_NAME here is
> name-length and type validation and name-extraction should not be separated.
> These are closely related, so these should be placed physically and 
> temporally close.

Please explain why you shouldn't validate TYPE_NAME here.


3. About using exfat_get_validated_dentry() in 
exfat_update_dir_chksum_with_entry_set()
Below are the changes in '[PATCH v4 1/5] exfat: integrates dir-entry getting 
and validation'
>   for (i = 0; i < es->num_entries; i++) {
> - ep = exfat_get_dentry_cached(es, i);
> + ep = exfat_get_validated_dentry(es, i, TYPE_ALL);
>   chksum = exfat_calc_chksum16(ep, DENTRY_SIZE, chksum,
>chksum_type);

Your request for this change is
> Ditto, You do not need to repeatedly call exfat_get_validated_dentry() for 
> the entries
> which got from exfat_get_dentry_set().

Even if the entry was got from exfat_get_dentry_set(), we need to get the ep 
again to calculate the checksum.
exfat_get_validated_dentry() with TYPE_ALL is the same as 
exfat_get_dentry_cached() because it allows all TYPEs.
Please elaborate on what the problem is.


4. About double-checking name entries as TYPE_SECONDARY and TYPE_NAME.
You said in 'RE: [PATCH v3] exfat: integrates dir-entry getting and validation'.
> your v3 patch are
> already checking the name entries as TYPE_SECONDARY. And it check them with
> TYPE_NAME again in exfat_get_uniname_from_ext_entry(). If you check TYPE_NAME
> with stream->name_len, We don't need to perform the loop for extracting
> filename from the name entries if stream->name_len or name entry is invalid.

It is rare case that stream->name_len or name-entry are invalid.
Perform the loop to extract filename when stream->name_len or name-entry is 
invalid has little effect.
What is the probrem with perform the loop for extract filename when 
stream->name_len or name-entry are invalid?


5. About validate flags as argument of 

RE: [PATCH 1/2] exfat: add NameLength check when extracting name

2020-08-17 Thread kohada.tetsuh...@dc.mitsubishielectric.co.jp
Thank you for your reply.

> > >> -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;
> > > Can we validate ->name_len and name entry ->type in 
> > > exfat_get_dentry_set() ?
> >
> > Yes.
> > As I wrote in a previous email, entry type validation, name-length
> > validation, and name extraction should not be separated, so implement all 
> > of these in exfat_get_dentry_set().
> > It can be easily implemented by adding uniname to
> > exfat_entry_set_cache and calling
> > exfat_get_uniname_from_name_entries() from exfat_get_dentry_set().
> No, We can check stream->name_len and name entry type in 
> exfat_get_dentry_set().

I have no objection to that point.

> And you are already checking entry type with TYPE_SECONDARY in 
> exfat_get_dentry_set(). Why do we have to check twice?

This verification is according to the description in '6.3 Generic Primary 
DirectoryEntry Template'.
The EntrySet is composed of one primary dir-entry and multiple Secondary 
dir-entries. 
We can validate the EntrySet by SecondaryCount and SetChecksum recorded in the 
Primary dir-entry.

The EntrySet checksum validation and dir-entries order validation are according 
to different descriptions. 
Therefore, it is not a double check.


> > However, that would be over-implementation.
> > Not all callers of exfat_get_dentry_set() need a name.
> Where? It will not checked with ES_2_ENTRIES.

The following functions don't require name.
__exfat_truncate()
__exfat_write_inode()
exfat_map_cluster()
exfat_find()


> > It is enough to validate the name when it is needed.
> > This is a file-system driver, not fsck.
> Sorry, I don't understand what you are talking about. If there is a problem 
> in ondisk-metadata, Filesystem should return
> error.

My explanation may have been inappropriate.
(Verifier is a better metaphor than fsck)
Essentially, the purpose of file-system driver is not to detect inconsistencies.
Of course, FSD should return error when it detects an inconsistency, as you say.
However, I think it is no-need for active inconsistency detection.


> > Validation is possible in exfat_get_dentry_set(), but unnecessary.
> >
> > Why do you want to validate the name in exfat_get_dentry_set()?
> exfat_get_dentry_set validates file, stream entry. 

> And you are trying to check name entries with type_secondary. 

It's a little different.
I'm trying to validate the checksum according to '6.3 Generic Primary 
DirectoryEntry Template'.

> In addition, trying add the checksum check.
> Conversely, Why would you want to add those checks to exfat_get_dentry_set()?

We should validate the EntrySet before using its contents.
(should not use contents of the EntrySet without validating it)
There are other filesystem designs that include crc/checksum in their each 
metadata headers.
Such a design can detect inconsistencies easily and effectively.
This verification is especially effective when meta-data is recorded in 
multiple sectors like the EntrySet.

> Why do we check only name entries separately? 

This verification is according to the description in '7.6.3 NameLength Field' 
and '7.7 File Name Directory Entry'.
Separated because it is  according to different description from checksum.
As I wrote before, I think TYPE_NAME and NameLength validation should not be 
separated from the name extraction.
(Because they are more strongly related than order of dir-entries)
So I think these should not be mixed with checksum verification.

When packing names into Name Dir-Entry...
We can also calculate the checksum while packing the name into the Name 
Dir-Entry.
However, we shouldn't mix them.


> Aren't you intent to return validated entry set in exfat_get_dentry_set()?

If so, add exfat_get_uniname_from_name_entries() after checksum 
verification.(as below)

/* EntrySet checksum verification */

+   if (max_entries == ES_ALL_ENTRIES &&
+   exfat_get_uniname_from_name_entries(es, es->uniname))
+   goto free_es;
return es;


The only callers that need a name are exfat_readdir() and 
exfat_find_dir_entry(), not the others.
Unnecessary name extraction violates the JIT principle.
(The size of exfat_entry_set_cache will also 

RE: [PATCH v2] exfat: integrates dir-entry getting and validation

2020-08-03 Thread kohada.tetsuh...@dc.mitsubishielectric.co.jp
Thank you for your reply.

> > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index
> > 573659bfbc55..09b85746e760 100644
> > --- a/fs/exfat/dir.c
> > +++ b/fs/exfat/dir.c
> > @@ -33,6 +33,7 @@ static void exfat_get_uniname_from_ext_entry(struct 
> > super_block *sb,  {
> > int i;
> > struct exfat_entry_set_cache *es;
> > +   struct exfat_dentry *ep;
> >
> > es = exfat_get_dentry_set(sb, p_dir, entry, ES_ALL_ENTRIES);
> > if (!es)
> > @@ -44,13 +45,9 @@ 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.
> >  */
> > -   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)
> > -   break;
> >
> > +   i = 2;
> > +   while ((ep = exfat_get_validated_dentry(es, i++, TYPE_NAME))) {
> As Sungjong said, I think that TYPE_NAME seems right to be validated in 
> exfat_get_dentry_set().

First, it is possible to correctly determine that 
"Immediately follow the Stream Extension directory entry as a consecutive 
series" 
whether the TYPE_NAME check is implemented here or exfat_get_dentry_set().
It's functionally same, so it is also right to validate in either.

Second, the current implementation does not care for NameLength field, as I 
replied to Sungjong.
If name is not terminated with zero, the name will be incorrect.(With or 
without my patch)
I think TYPE_NAME and NameLength validation should not be separated from the 
name extraction.
If validate TYPE_NAME in exfat_get_dentry_set(), NameLength validation and name 
extraction 
should also be implemented there.
(Otherwise, a duplication check with exfat_get_dentry_set() and here.)
I will add NameLength validation here.
Therefore, TYPE_NAME validation here should not be omitted.

Third, getting dentry and entry-type validation should be integrated.
These no longer have to be primitive.
The integration simplifies caller error checking.


> > -struct exfat_dentry *exfat_get_dentry_cached(
> > -   struct exfat_entry_set_cache *es, int num)
> > +struct exfat_dentry *exfat_get_validated_dentry(struct 
> > exfat_entry_set_cache *es,
> > +   int num, unsigned int type)
> Please use two tabs.

OK.
I'll fix it.


> > +   struct buffer_head *bh;
> > +   struct exfat_dentry *ep;
> >
> > -   return (struct exfat_dentry *)p;
> > +   if (num >= es->num_entries)
> > +   return NULL;
> > +
> > +   bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)];
> > +   if (!bh)
> > +   return NULL;
> > +
> > +   ep = (struct exfat_dentry *)
> > +   (bh->b_data + EXFAT_BLK_OFFSET(off, es->sb));
> > +
> > +   switch (type) {
> > +   case TYPE_ALL: /* accept any */
> > +   break;
> > +   case TYPE_FILE:
> > +   if (ep->type != EXFAT_FILE)
> > +   return NULL;
> > +   break;
> > +   case TYPE_SECONDARY:
> > +   if (!(type & exfat_get_entry_type(ep)))
> > +   return NULL;
> > +   break;
> Type check should be in this order : 
> FILE->STREAM->NAME->{CRITICAL_SEC|BENIGN_SEC}
> I think that you are missing TYPE_NAME check here.

Types other than the above (TYPE_NAME, TYPE_STREAM, etc.) are checked in the 
default-case(as below).

> > +   default:
> > +   if (type != exfat_get_entry_type(ep))
> > +   return NULL;
> > +   }
> > +   return ep;
> >  }
> >
> >  /*
> >   * Returns a set of dentries for a file or dir.
> >   *
> > - * Note It provides a direct pointer to bh->data via 
> > exfat_get_dentry_cached().
> > + * Note It provides a direct pointer to bh->data via 
> > exfat_get_validated_dentry().
> >   * User should call exfat_get_dentry_set() after setting 'modified' to 
> > apply
> >   * changes made in this entry set to the real device.
> >   *
> >   * in:
> >   *   sb+p_dir+entry: indicates a file/dir
> > - *   type:  specifies how many dentries should be included.
> > + *   max_entries:  specifies how many dentries should be included.
> >   * return:
> >   *   pointer of entry set on success,
> >   *   NULL on failure.
> > + * note:
> > + *   On success, guarantee the correct 'file' and 'stream-ext' dir-entries.
> This comment seems unnecessary.

I'll remove it.

> > diff --git a/fs/exfat/file.c b/fs/exfat/file.c index
> > 6707f3eb09b5..b6b458e6f5e3 100644
> > --- a/fs/exfat/file.c
> > +++ b/fs/exfat/file.c
> > @@ -160,8 +160,8 @@ int __exfat_truncate(struct inode *inode, loff_t 
> > new_size)
> > ES_ALL_ENTRIES);
> > if (!es)
> > return -EIO;
> > -   ep = exfat_get_dentry_cached(es, 0);
> > -   ep2 = exfat_get_dentry_cached(es, 1);
> > +   ep = exfat_get_validated_dentry(es, 0, TYPE_FILE);
> > +   ep2 = 

RE: [RFC]PATCH] exfat: integrates dir-entry getting and validation

2020-07-13 Thread kohada.tetsuh...@dc.mitsubishielectric.co.jp
Thanks for your reply.

> >
> > /* 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.

OK. I'll change that. 
However, this for-loop is considering changing to checksum verification.


> 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.

I think it is appropriate as a check here.
TYPE_NAME starting without index=2 doesn't accept in 
exfat_get_uniname_from_ext_entry().
However, I think this is not enough.
This is because there is no check for name-length. (same with or without patch)

I will check the name-length in the next(or after next) patch.


BR
---
Kohada Tetsuhiro 


RE: [PATCH] exfat: retain 'VolumeFlags' properly

2020-07-13 Thread kohada.tetsuh...@dc.mitsubishielectric.co.jp
Thanks for your reply.

> > Also, rename ERR_MEDIUM to MED_FAILURE.
> I think that MEDIA_FAILURE looks better.

I think so too.
If so, should I change VOL_DIRTY to VOLUME_DIRTY?

> > -int exfat_set_vol_flags(struct super_block *sb, unsigned short
> > new_flag)
> > +int exfat_set_vol_flags(struct super_block *sb, unsigned short
> > +new_flags)
> >  {
> > struct exfat_sb_info *sbi = EXFAT_SB(sb);
> > struct boot_sector *p_boot = (struct boot_sector *)sbi->boot_bh->b_data;
> > bool sync;
> If dirty bit is set in on-disk volume flags, we can just return 0 at the 
> beginning of this function ?

That's right.
Neither 'set VOL_DIRTY' nor 'set VOL_CLEAN' makes a change to volume flags.


> > +   if (new_flags == VOL_CLEAN)
> > +   new_flags = (sbi->vol_flags & ~VOL_DIRTY) | 
> > sbi->vol_flags_noclear;
> > +   else
> > +   new_flags |= sbi->vol_flags;
> > +
> > /* flags are not changed */
> > -   if (sbi->vol_flag == new_flag)
> > +   if (sbi->vol_flags == new_flags)
> > return 0;
> >
> > -   sbi->vol_flag = new_flag;
> > +   sbi->vol_flags = new_flags;
> >
> > /* skip updating volume dirty flag,
> >  * if this volume has been mounted with read-only @@ -114,9 +119,9
> > @@ int exfat_set_vol_flags(struct super_block *sb, unsigned short new_flag)
> > if (sb_rdonly(sb))
> > return 0;
> >
> > -   p_boot->vol_flags = cpu_to_le16(new_flag);
> > +   p_boot->vol_flags = cpu_to_le16(new_flags);
> How about set or clear only dirty bit to on-disk volume flags instead of using
> sbi->vol_flags_noclear ?
>if (set)
>p_boot->vol_flags |= cpu_to_le16(VOL_DIRTY);
>else
>p_boot->vol_flags &= cpu_to_le16(~VOL_DIRTY);

In this way, the initial  VOL_DIRTY cannot be retained.
The purpose of this patch is to avoid losing the initial VOL_DIRTY and 
MED_FAILURE.
Furthermore, I'm also thinking of setting MED_FAILURE.

However, the formula for 'new_flags' may be a bit complicated.
Is it better to change to the following?

if (new_flags == VOL_CLEAN)
new_flags = sbi->vol_flags & ~VOL_DIRTY
else
new_flags |= sbi->vol_flags;

new_flags |= sbi->vol_flags_noclear;


one more point,
Is there a better name than 'vol_flags_noclear'?
(I can't come up with a good name anymore)

BR
---
Kohada Tetsuhiro 


RE: [PATCH 1/2 v4] exfat: write multiple sectors at once

2020-06-21 Thread kohada.tetsuh...@dc.mitsubishielectric.co.jp
> 2020-06-19 17:38 GMT+09:00, Tetsuhiro Kohada :
> > Write multiple sectors at once when updating dir-entries.
> > Add exfat_update_bhs() for that. It wait for write completion once
> > instead of sector by sector.
> > It's only effective if sync enabled.
> >
> > Reviewed-by: Christoph Hellwig 
> He didn't give reviewed-by tag for this patch.
> You shouldn't add it at will.

I understand.
Should I remove reviewed-by tag and repost the patch?

BR
---
Kohada Tetsuhiro 


RE: [PATCH] exfat: remove EXFAT_SB_DIRTY flag

2020-06-15 Thread kohada.tetsuh...@dc.mitsubishielectric.co.jp
> > 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.

OK.
I will repost v2-patch with the 'wait' check added.
Thanks for your comment.


> > >> +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
---
Kohada Tetsuhiro 


RE: [PATCH 3/3] exfat: set EXFAT_SB_DIRTY and VOL_DIRTY at the same timing

2020-06-07 Thread kohada.tetsuh...@dc.mitsubishielectric.co.jp
Thank you for your comment.

> >> Can you split this patch into two? (Don't set VOL_DIRTY on -ENOTEMPTY and
> >> Setting EXFAT_SB_DIRTY is
> >> merged into exfat_set_vol_flag). I need to check the second one more.
> >
> > Can't do that.
> >
> > exfat_set_vol_flag() is called when rmdir processing begins. When Not-empty
> > is detected,
> > VOL_DIRTY has already been written and synced to the media.
> You can move it before calling exfat_remove_entries().

Can be moved, but that doesn't solve the problem.
It causes the similar problem as before.

exfat_remove_entries() calls exfat_get_dentry().
If exfat_get_dentry() fails, update bh and set SB_DIRTY will not be executed.
As a result, SB_DIRTY is not set and sync does not work.
Similar problems occur with other writing functions.
Similar problems occur when pre-write checks are added in the future.

If you don't set VOL_DIRTY at the beginning, you should delay to set VOL_DIRTY 
until update-bh & set SB_DIRTY.
This avoids unnecessary changes to VOL_DIRTY/VOL_CLEAN.
I think this method is smart, but it is difficult to decide when to set 
VOL_CLEAN.
(I tried to implement it, but gave up)

> > By doing this, sync is guaranteed if VOL_DIRTY is set by calling
> > exfat_set_vol_flag.
> >
> > This change may still have problems, but it's little better than before, I
> > think.
> I need to check more if it is the best or there is more better way.

I think the sync-problems still exist.
Let's improve little by little. :-)

BR
---
Kohada Tetsuhiro 


Re: [PATCH] exfat: optimize dir-cache

2020-05-27 Thread 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?

BR
---
Kohada Tetsuhiro