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-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
