Re: [PATCH] direct-io: fix stale data exposure from concurrent buffered read

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, May 06, 2016 at 10:13:39AM -0400, Jeff Moyer wrote:
> Eryu Guan <guaneryu@xxxxxxxxx> writes:
> 
> > On Thu, May 05, 2016 at 03:39:29PM -0400, Jeff Moyer wrote:
> >> I think this code operates on blocks for a reason: we're trying to
> >> determine if we'll trigger block allocation, right?  For example,
> >> consider a sparse file with i_size of 2k, and a write to offset 2k into
> >> the file, with a file system block size of 4k.  Should that have create
> >> set or not?
> >
> > Thanks for pointing this out! I think 'create' should be 0 in this case,
> > my test failed in this case, with both 4.6-rc6 stock kernel and my
> > patched kernel.
> >
> > I'm testing an updated patch now, hopefully it's doing the right thing.
> > It's basiclly something like:
> >
> > if (offset < i_size)
> > 	create = 0;
> > else if ((block_in_file >> blkfactor) == (i_size >> (blkbits + blkfactor)) &&
> > 	 (i_size & ((1 << (blkbits + blkfactor)) - 1)))
> > 	create = 0;
> 
> I think that can be simplified to a single check;  something like:
> 
> 	if (block_in_file < total_blocks_in_file)
> 		create = 0;

I may miss something, but this doesn't seem right to me. Still take your
example, on a 4k block size & 512 sector size filesystem

	xfs_io -f -c "truncate 2k" testfile
	xfs_io -d -c "pwrite 2k 2k" testfile

block_in_file is 4 (dio block size is 512 in this case, 4 blocks for 2k
size), total_blocks_in_file is 0, so 'create' is set, but it should be 0

> 
> >> > Also introduce some local variables to make the code
> >> > easier to read a little bit.
> >> 
> >> Please don't do this.  You're only making the change harder to review.
> >> Just submit the minimal fix.  You can submit cleanups as a follow-on.
> >
> > I think it's not a pure cleanup, it's needed as things like
> > 'sdio->block_in_file' are referenced multiple times in the function, and
> > they are making the lines too long to read/write. Maybe I should have
> > made it clear in the first place.
> 
> I still view that as a cleanup.  If you had submitted the minimal patch,
> I would have to look at a couple lines of change.  In code this tricky,
> I'd rather not have to stare at all the code movement to make sure
> you got that part right, too.
> 
> But do what you feel is right, I'll review it either way.  ;-)

Thanks very much! I'll split it to two patches, first one is a cleanup,
has no function change, second one is the real fix. This should make the
review easier.

Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux