On Tue, Aug 13, 2019 at 05:57:22PM +0530, RITESH HARJANI wrote: > On 8/13/19 4:40 PM, Matthew Bobrowski wrote: > > On Mon, Aug 12, 2019 at 11:01:50PM +0530, RITESH HARJANI wrote: > > > I was under the assumption that we need to maintain > > > ext4_test_inode_state(inode, EXT4_STATE_DIO_UNWRITTEN) or > > > atomic_read(&EXT4_I(inode)->i_unwritten)) > > > in case of non-AIO directIO or AIO directIO case as well (when we may > > > allocate unwritten extents), > > > to protect with some kind of race with other parts of code(maybe > > > truncate/bufferedIO/fallocate not sure?) which may call for > > > ext4_can_extents_be_merged() > > > to check if extents can be merged or not. > > > > > > Is it not the case? > > > Now that directIO code has no way of specifying that this inode has > > > unwritten extent, will it not race with any other path, where this info was > > > necessary (like > > > in above func ext4_can_extents_be_merged())? > > Ah yes, I was under the same assumption when reviewing the code > > initially and one of my first solutions was to also use this dynamic > > 'state' flag in the ->end_io() handler. But, I fell flat on my face as > > that deemed to be problematic... This is because there can be multiple > > direct IOs to unwritten extents against the same inode, so you cannot > > possibly get away with tracking them using this single inode flag. So, > > hence the reason why we drop using EXT4_STATE_DIO_UNWRITTEN and use > > IOMAP_DIO_UNWRITTEN instead in the ->end_io() handler, which tracks > > whether _this_ particular IO has an underlying unwritten extent. > > Thanks for taking time to explain this. > > But what I meant was this (I may be wrong here since I haven't really looked > into it), > but for my understanding I would like to discuss this - > > So earlier with this flag(EXT4_STATE_DIO_UNWRITTEN) we were determining on > whether a newextent can be merged with ex1 in function > ext4_extents_can_be_merged. But now since we have removed that flag we have > no way of knowing that whether > this inode has any unwritten extents or not from any DIO path. > What I meant is isn't this removal of setting/unsetting of > flag(EXT4_STATE_DIO_UNWRITTEN) > changing the behavior of this func - ext4_extents_can_be_merged? Ah yes, I see. I believe that what you're saying is correct and I think we will need to account for this case. But, I haven't thought about how to do this just yet. > Also - could you please explain why this check returns 0 in the first place > (line 1762 - 1766 below)? I cannot explain why, because I myself don't know exactly why in this particular case the extents cannot be merged. Perhaps `git blame` is our friend and we can direct that question accordingly, or someone else on this mailing list knows the answer. :-) > 1733 int > 1734 ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent > *ex1, > 1735 struct ext4_extent *ex2) > <...> > > 1762 if (ext4_ext_is_unwritten(ex1) && > 1763 (ext4_test_inode_state(inode, EXT4_STATE_DIO_UNWRITTEN) || > 1764 atomic_read(&EXT4_I(inode)->i_unwritten) || > 1765 (ext1_ee_len + ext2_ee_len > EXT_UNWRITTEN_MAX_LEN))) > 1766 return 0; --M
