[snip]
> > > + if (pos > valid_size && iocb_is_dsync(iocb)) {
> > > + ssize_t err = vfs_fsync_range(file, valid_size, pos - 1,
> > > + iocb->ki_flags & IOCB_SYNC);
> > If there is a hole between valid_size and pos, it seems to call sync
> twice.
> > Is there any reason to call separately?
> > Why don't you call the vfs_fsync_range only once for the merged scope
> > [valid_size:end]?
>
> For better debugging, I kept the original logic and added new logic for
> valid_size.
> For now, it is unnecessary, I will change to sync once.
Thanks.
>
> >
[snip]
> > Is there any reason to not only change the value but also move the line
> down?
>
> I will move it back the original line.
Sounds good!
>
> >
> > > +
> > > exfat_update_dir_chksum_with_entry_set(&es);
> > > return exfat_put_dentry_set(&es, sync); } @@ -306,17 +307,25 @@
> > > static int exfat_get_block(struct inode *inode, sector_t iblock,
> > > mapped_blocks = sbi->sect_per_clus - sec_offset;
> > > max_blocks = min(mapped_blocks, max_blocks);
> > >
> > > - /* Treat newly added block / cluster */
> > > - if (iblock < last_block)
> > > - create = 0;
> > > -
> > > - if (create || buffer_delay(bh_result)) {
> > > - pos = EXFAT_BLK_TO_B((iblock + 1), sb);
> > > + pos = EXFAT_BLK_TO_B((iblock + 1), sb);
> > > + if ((create && iblock >= last_block) || buffer_delay(bh_result)) {
> > > if (ei->i_size_ondisk < pos)
> > > ei->i_size_ondisk = pos;
> > > }
> > >
> > > + map_bh(bh_result, sb, phys);
> > > + if (buffer_delay(bh_result))
> > > + clear_buffer_delay(bh_result);
> > > +
> > > if (create) {
> > > + sector_t valid_blks;
> > > +
> > > + valid_blks = EXFAT_B_TO_BLK_ROUND_UP(ei->valid_size, sb);
> > > + if (iblock < valid_blks && iblock + max_blocks >= valid_blks)
> > > {
> > > + max_blocks = valid_blks - iblock;
> > > + goto done;
> > > + }
> > > +
> > You removed the code for handling the case for (iblock < last_block).
> > So, under all write call-flows, it could be buffer_new abnormally.
> > It seems wrong. right?
>
> Yes, I will update this patch.
>
> Without this patch, last_block is equal with valid_blks,
> exfat_map_new_buffer() should be called if iblock + max_blocks >
> last_block.
>
> With this patch, last_block >= valid_blks, exfat_map_new_buffer() should
> be called if iblock + max_blocks > valid_blks.
Okay.
>
> >
> > > err = exfat_map_new_buffer(ei, bh_result, pos);
> > > if (err) {
> > > exfat_fs_error(sb,
> > [snip]
> > > @@ -436,8 +485,20 @@ static ssize_t exfat_direct_IO(struct kiocb
> > > *iocb, struct iov_iter *iter)
> > > * condition of exfat_get_block() and ->truncate().
> > > */
> > > ret = blockdev_direct_IO(iocb, inode, iter, exfat_get_block);
> > > - if (ret < 0 && (rw & WRITE))
> > > - exfat_write_failed(mapping, size);
> > > + if (ret < 0) {
> > > + if (rw & WRITE)
> > > + exfat_write_failed(mapping, size);
> > > +
> > > + if (ret != -EIOCBQUEUED)
> > > + return ret;
> > > + } else
> > > + size = pos + ret;
> > > +
> > > + if ((rw & READ) && pos < ei->valid_size && ei->valid_size < size) {
> > > + iov_iter_revert(iter, size - ei->valid_size);
> > > + iov_iter_zero(size - ei->valid_size, iter);
> > > + }
> >
> > This approach causes unnecessary reads to the range after valid_size,
> right?
>
> I don't think so.
>
> If the blocks across valid_size, the iov_iter will be handle as 1. Read
> the blocks before valid_size.
> 2. Read the block where valid_size is located and set the area after
> valid_size to zero.
> 3. zero the buffer of the blocks after valid_size(not read from disk)
>
> So there are unnecessary zeroing here(in 1 and 2), no unnecessary reads.
> I will remove the unnecessary zeroing.
You are right. There might be no need to change.
It could be handled in do_direct_IO() with get_block newly modifed.
Thanks.
B. R.
Sungjong Seo
