| Message ID | 20201012092938.50946-9-chandanrlinux@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bail out if transaction can cause extent count to overflow | expand |
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.
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.
On Thu, Oct 15, 2020 at 03:31:26PM +0530, Chandan Babu R wrote: > 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. Probably not a huge deal, since at worst we bail out of reflink early and userspace can just decide to fall back to a pagecache copy or whatever. It'd be harder to deal with if this was the cow path where we long ago returned from write() and even writeback... ...though now that I think about it, what /does/ happens if _reflink_end_cow trips over this? I wonder if inodes need to be able to reserve extent count for later, but ... hgnghghg that seems hard to reason about. --D > > > -- > chandan > > >
On Friday 16 October 2020 12:15:45 AM IST Darrick J. Wong wrote: > On Thu, Oct 15, 2020 at 03:31:26PM +0530, Chandan Babu R wrote: > > 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. > > Probably not a huge deal, since at worst we bail out of reflink early > and userspace can just decide to fall back to a pagecache copy or > whatever. It'd be harder to deal with if this was the cow path where we > long ago returned from write() and even writeback... > > ...though now that I think about it, what /does/ happens if > _reflink_end_cow trips over this? I wonder if inodes need to be able to > reserve extent count for later, but ... hgnghghg that seems hard to > reason about. I am not sure if I am following you. Looks like you are asking about what happens if xfs_reflink_end_cow_extent() trips over XFS_IEXT_REFLINK_END_COW_CNT limit. If that occurs, then the end_io path would set AS_EIO on mapping->flags which would in turn cause the upper layers of the kernel to return -EIO to the corresponding fsync() call made by a userspace program.
On Thu, Oct 15, 2020 at 03:31:26PM +0530, Chandan Babu R wrote: > 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. No, I think the dynamic value is perfectly fine, as we have all the information trivially available. I just don't think having a separate macro and the comment explaining it away from the actual functionality is helpful.
On Friday 16 October 2020 12:34:48 PM IST Christoph Hellwig wrote: > On Thu, Oct 15, 2020 at 03:31:26PM +0530, Chandan Babu R wrote: > > 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. > > No, I think the dynamic value is perfectly fine, as we have all the > information trivially available. I just don't think having a separate > macro and the comment explaining it away from the actual functionality > is helpful. > Darrick, I think using the macros approach is more suitable. But I can go ahead and implement the approach decided by the community. Please let me know your opinion.
On Fri, Oct 16, 2020 at 04:58:53PM +0530, Chandan Babu R wrote: > On Friday 16 October 2020 12:34:48 PM IST Christoph Hellwig wrote: > > On Thu, Oct 15, 2020 at 03:31:26PM +0530, Chandan Babu R wrote: > > > 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. > > > > No, I think the dynamic value is perfectly fine, as we have all the > > information trivially available. I just don't think having a separate > > macro and the comment explaining it away from the actual functionality > > is helpful. > > > > Darrick, I think using the macros approach is more suitable. But I can go > ahead and implement the approach decided by the community. Please let me know > your opinion. The macro only gets used in one place anyway, so I don't see as strong a need for it as the other places. I think this one could be open-coded next to the places where we decide the values of smap_real and dmap_written. (i.e. what Christoph is suggesting) --D > -- > chandan > > >
On Friday 16 October 2020 8:59:00 PM IST Darrick J. Wong wrote: > On Fri, Oct 16, 2020 at 04:58:53PM +0530, Chandan Babu R wrote: > > On Friday 16 October 2020 12:34:48 PM IST Christoph Hellwig wrote: > > > On Thu, Oct 15, 2020 at 03:31:26PM +0530, Chandan Babu R wrote: > > > > 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. > > > > > > No, I think the dynamic value is perfectly fine, as we have all the > > > information trivially available. I just don't think having a separate > > > macro and the comment explaining it away from the actual functionality > > > is helpful. > > > > > > > Darrick, I think using the macros approach is more suitable. But I can go > > ahead and implement the approach decided by the community. Please let me know > > your opinion. > > The macro only gets used in one place anyway, so I don't see as strong a > need for it as the other places. I think this one could be open-coded > next to the places where we decide the values of smap_real and > dmap_written. (i.e. what Christoph is suggesting) > Ok. I will make the relevant changes.
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h index b99e67e7b59b..ded3c1b56c94 100644 --- a/fs/xfs/libxfs/xfs_inode_fork.h +++ b/fs/xfs/libxfs/xfs_inode_fork.h @@ -87,6 +87,21 @@ struct xfs_ifork { */ #define XFS_IEXT_REFLINK_END_COW_CNT (2) +/* + * 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; + if (smap_real) { /* * If the extent we're unmapping is backed by storage (written