On Thursday 15 October 2020 2:09:45 PM IST Christoph Hellwig wrote: > This patch demonstrates very well why I think having these magic > defines and the comments in a header makes no sense. > > > +/* > > + * Remapping an extent involves unmapping the existing extent and mapping in the > > + * new extent. > > + * > > + * When unmapping, an extent containing the entire unmap range can be split into > > + * two extents, > > + * i.e. | Old extent | hole | Old extent | > > + * Hence extent count increases by 1. > > + * > > + * Mapping in the new extent into the destination file can increase the extent > > + * count by 1. > > + */ > > +#define XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written) \ > > + (((smap_real) ? 1 : 0) + ((dmap_written) ? 1 : 0)) > > + > > /* > > * Fork handling. > > */ > > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c > > index 4f0198f636ad..c9f9ff68b5bb 100644 > > --- a/fs/xfs/xfs_reflink.c > > +++ b/fs/xfs/xfs_reflink.c > > @@ -1099,6 +1099,11 @@ xfs_reflink_remap_extent( > > goto out_cancel; > > } > > > > + error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK, > > + XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written)); > > + if (error) > > + goto out_cancel; > > + > > This is a completely mess. > > If OTOH xfs_reflink_remap_extent had a local variable for the potential > max number of extents, which is incremented near the initialization > of smap_real and dmap_written, with a nice comment near to each > increment it would make complete sense to the reader. > How about following the traits of XFS_IEXT_WRITE_UNWRITTEN_CNT (writing to unwritten extent) and XFS_IEXT_REFLINK_END_COW_CNT (moving an extent from cow fork to data fork) and setting XFS_IEXT_REFLINK_REMAP_CNT to a worst case value of 2? A write spanning the entirety of an unwritten extent does not change the extent count. Similarly, If there are no extents in the data fork spanning the file range mapped by an extent in the cow fork, moving the extent from cow fork to data fork increases the extent count by just 1 and not by the worst case count of 2. -- chandan
