[V2,08/10] xfs: Check for extent overflow when moving extent from cow to data fork
diff mbox series

Message ID 20200814080833.84760-9-chandanrlinux@gmail.com
State Superseded
Headers show
Series
  • Bail out if transaction can cause extent count to overflow
Related show

Commit Message

Chandan Babu R Aug. 14, 2020, 8:08 a.m. UTC
Moving an extent to data fork can cause a sub-interval of an existing
extent to be unmapped. This will increase extent count by 1. Mapping in
the new extent can increase the extent count by 1 again i.e.
 | Old extent | New extent | Old extent |
Hence number of extents increases by 2.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_inode_fork.h | 10 +++++++++-
 fs/xfs/xfs_reflink.c           |  6 ++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Darrick J. Wong Aug. 18, 2020, 10:03 p.m. UTC | #1
On Fri, Aug 14, 2020 at 01:38:31PM +0530, Chandan Babu R wrote:
> Moving an extent to data fork can cause a sub-interval of an existing
> extent to be unmapped. This will increase extent count by 1. Mapping in
> the new extent can increase the extent count by 1 again i.e.
>  | Old extent | New extent | Old extent |
> Hence number of extents increases by 2.
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_inode_fork.h | 10 +++++++++-
>  fs/xfs/xfs_reflink.c           |  6 ++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 63f83a13e0a8..d750bdff17c9 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -76,7 +76,15 @@ struct xfs_ifork {
>   * increase by 1.
>   */
>  #define XFS_IEXT_INSERT_HOLE_CNT 1
> -
> +/*
> + * Moving an extent to data fork can cause a sub-interval of an
> + * existing extent to be unmapped. This will increase extent count by
> + * 1. Mapping in the new extent can increase the extent count by 1
> + * again i.e.
> + * | Old extent | New extent | Old extent |

This comment is a little oddly formatted, mostly because my brain
thought that the line starting with "1. Mapping" was a numbered bullet
list.  If you reflow the comment further outward, you can get it to look
like this:

/*
 * Moving an extent to data fork can cause a sub-interval of an existing
 * extent to be unmapped, increasing extent count by 1. Mapping in the
 * new extent can also increase the extent count by 1:
 * | Old extent | New extent | Old extent |
 * Hence number of extents increases by 2.
 */
#define XFS_IEXT_REFLINK_END_COW_CNT 2

--D

> + * Hence number of extents increases by 2.
> + */
> +#define XFS_IEXT_REFLINK_END_COW_CNT 2
>  /*
>   * Fork handling.
>   */
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index aac83f9d6107..04a7754ee681 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -29,6 +29,7 @@
>  #include "xfs_iomap.h"
>  #include "xfs_sb.h"
>  #include "xfs_ag_resv.h"
> +#include "xfs_trans_resv.h"
>  
>  /*
>   * Copy on Write of Shared Blocks
> @@ -628,6 +629,11 @@ xfs_reflink_end_cow_extent(
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	xfs_trans_ijoin(tp, ip, 0);
>  
> +	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> +			XFS_IEXT_REFLINK_END_COW_CNT);
> +	if (error)
> +		goto out_cancel;
> +
>  	/*
>  	 * In case of racing, overlapping AIO writes no COW extents might be
>  	 * left by the time I/O completes for the loser of the race.  In that
> -- 
> 2.28.0
>
Chandan Babu R Aug. 19, 2020, 5:04 a.m. UTC | #2
On Wednesday 19 August 2020 3:33:07 AM IST Darrick J. Wong wrote:
> On Fri, Aug 14, 2020 at 01:38:31PM +0530, Chandan Babu R wrote:
> > Moving an extent to data fork can cause a sub-interval of an existing
> > extent to be unmapped. This will increase extent count by 1. Mapping in
> > the new extent can increase the extent count by 1 again i.e.
> >  | Old extent | New extent | Old extent |
> > Hence number of extents increases by 2.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_fork.h | 10 +++++++++-
> >  fs/xfs/xfs_reflink.c           |  6 ++++++
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 63f83a13e0a8..d750bdff17c9 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -76,7 +76,15 @@ struct xfs_ifork {
> >   * increase by 1.
> >   */
> >  #define XFS_IEXT_INSERT_HOLE_CNT 1
> > -
> > +/*
> > + * Moving an extent to data fork can cause a sub-interval of an
> > + * existing extent to be unmapped. This will increase extent count by
> > + * 1. Mapping in the new extent can increase the extent count by 1
> > + * again i.e.
> > + * | Old extent | New extent | Old extent |
> 
> This comment is a little oddly formatted, mostly because my brain
> thought that the line starting with "1. Mapping" was a numbered bullet
> list.  If you reflow the comment further outward, you can get it to look
> like this:
> 
> /*
>  * Moving an extent to data fork can cause a sub-interval of an existing
>  * extent to be unmapped, increasing extent count by 1. Mapping in the
>  * new extent can also increase the extent count by 1:
>  * | Old extent | New extent | Old extent |
>  * Hence number of extents increases by 2.
>  */
> #define XFS_IEXT_REFLINK_END_COW_CNT 2
>

Sure. I will fix up all the comments in the patchset to extend upto 80
columns.

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 63f83a13e0a8..d750bdff17c9 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -76,7 +76,15 @@  struct xfs_ifork {
  * increase by 1.
  */
 #define XFS_IEXT_INSERT_HOLE_CNT 1
-
+/*
+ * Moving an extent to data fork can cause a sub-interval of an
+ * existing extent to be unmapped. This will increase extent count by
+ * 1. Mapping in the new extent can increase the extent count by 1
+ * again i.e.
+ * | Old extent | New extent | Old extent |
+ * Hence number of extents increases by 2.
+ */
+#define XFS_IEXT_REFLINK_END_COW_CNT 2
 /*
  * Fork handling.
  */
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index aac83f9d6107..04a7754ee681 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -29,6 +29,7 @@ 
 #include "xfs_iomap.h"
 #include "xfs_sb.h"
 #include "xfs_ag_resv.h"
+#include "xfs_trans_resv.h"
 
 /*
  * Copy on Write of Shared Blocks
@@ -628,6 +629,11 @@  xfs_reflink_end_cow_extent(
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	xfs_trans_ijoin(tp, ip, 0);
 
+	error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
+			XFS_IEXT_REFLINK_END_COW_CNT);
+	if (error)
+		goto out_cancel;
+
 	/*
 	 * In case of racing, overlapping AIO writes no COW extents might be
 	 * left by the time I/O completes for the loser of the race.  In that