On 2024/4/29 18:25, Jan Kara wrote: > On Fri 26-04-24 17:38:23, Zhang Yi wrote: >> On 2024/4/25 23:56, Jan Kara wrote: >>> On Wed 10-04-24 11:41:57, Zhang Yi wrote: >>>> From: Zhang Yi <yi.zhang@xxxxxxxxxx> >>>> >>>> The cached delalloc or hole extent should be trimed to the map->map_len >>>> if we map delalloc blocks in ext4_da_map_blocks(). But it doesn't >>>> trigger any issue now because the map->m_len is always set to one and we >>>> always insert one delayed block once a time. Fix this by trim the extent >>>> once we get one from the cached extent tree, prearing for mapping a >>>> extent with multiple delalloc blocks. >>>> >>>> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx> >>> >>> Well, but we already do the trimming in ext4_da_map_blocks(), don't we? You >>> just move it to a different place... Or do you mean that we actually didn't >>> set 'map' at all in some cases and now we do? >> >> Yeah, now we only trim map len if we found an unwritten extent or written >> extent in the cache, this isn't okay if we found a hole and >> ext4_insert_delayed_block() and ext4_da_map_blocks() support inserting >> map->len blocks. If we found a hole which es->es_len is shorter than the >> length we want to write, we could delay more blocks than we expected. >> >> Please assume we write data [A, C) to a file that contains a hole extent >> [A, B) and a written extent [B, D) in cache. >> >> A B C D >> before da write: ...hhhhhh|wwwwww.... >> >> Then we will get extent [A, B), we should trim map->m_len to B-A before >> inserting new delalloc blocks, if not, the range [B, C) is duplicated. > > Thanks for explanation! > Current change log is not clear enough now, I will put this explanation into the changelog in my nextn iteratio, make it easier to understand. >>> In either case the 'map' >>> handling looks a bit sloppy in ext4_da_map_blocks() as e.g. the >>> 'add_delayed' case doesn't seem to bother with properly setting 'map' based >>> on what it does. So maybe we should clean that up to always set 'map' just >>> before returning at the same place where we update the 'bh'? And maybe bh >>> update could be updated in some common helper because it's content is >>> determined by the 'map' content? >>> >> >> I agree with you, it looks that we should always revise the map->m_len >> once we found an extent from the cache, and then do corresponding handling >> according to the extent type. so it's hard to put it to a common place. >> But we can merge the handling of written and unwritten extent, I've moved >> the bh updating into ext4_da_get_block_prep() and do some cleanup in >> patch 9, please look at that patch, does it looks fine to you? > > Oh, yes, what patch 9 does improve things significantly and it addresses my > concern. So feel free to add: > > Reviewed-by: Jan Kara <jack@xxxxxxx> > > Maybe in the changelog you can just mention that the remaining cases not > setting map->m_len will be handled in patch 9. > Sure. Thanks, Yi.
