RE: [PATCH v4 1/2] exfat: change to get file size from DataLength

["Sungjong Seo" <sj1557.seo@samsung.com>]

[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