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! > > 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. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
