Btrfs devel - Aug 2011 - [PATCH] Btrfs: reserve sufficient space for ioctl clone

Fix a crash/BUG_ON in the clone ioctl due to insufficient reservation. We
need to reserve space for:

 - adjusting the old extent (possibly splitting it)
 - adding the new extent
 - updating the inode

Signed-off-by: Sage Weil <sage@newdream.net>
---
 fs/btrfs/ioctl.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a3c4751..f038d4a 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2320,7 +2320,12 @@ static noinline long btrfs_ioctl_clone(struct file *file,
unsigned long srcfd,
 			else
 				new_key.offset = destoff;
 
-			trans = btrfs_start_transaction(root, 1);
+			/*
+			 * 1 - adjusting old extent (we may have to split it)
+			 * 1 - add new extent
+			 * 1 - inode update
+			 */
+			trans = btrfs_start_transaction(root, 3);
 			if (IS_ERR(trans)) {
 				ret = PTR_ERR(trans);
 				goto out;
-- 
1.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On tue, 9 Aug 2011 10:46:37 -0700, Sage Weil wrote:
> Fix a crash/BUG_ON in the clone ioctl due to insufficient reservation. We
> need to reserve space for:
> 
>  - adjusting the old extent (possibly splitting it)
>  - adding the new extent
>  - updating the inode
> 
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  fs/btrfs/ioctl.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a3c4751..f038d4a 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2320,7 +2320,12 @@ static noinline long btrfs_ioctl_clone(struct file
*file, unsigned long srcfd,
>  			else
>  				new_key.offset = destoff;
>  
> -			trans = btrfs_start_transaction(root, 1);
> +			/*
> +			 * 1 - adjusting old extent (we may have to split it)
I don''t think it is enough. If we have lots of file extents and their
extent items are stored
in many contiguous leaves, the drop operation may cause those leaves to be
COWed. So I think we
must calculate the number of leaves which will be COWed at first.

Thanks
Miao
> +			 * 1 - add new extent
> +			 * 1 - inode update
> +			 */
> +			trans = btrfs_start_transaction(root, 3);
>  			if (IS_ERR(trans)) {
>  				ret = PTR_ERR(trans);
>  				goto out;
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 16 Aug 2011, Miao Xie wrote:
> On tue, 9 Aug 2011 10:46:37 -0700, Sage Weil wrote:
> > Fix a crash/BUG_ON in the clone ioctl due to insufficient reservation.
We
> > need to reserve space for:
> > 
> >  - adjusting the old extent (possibly splitting it)
> >  - adding the new extent
> >  - updating the inode
> > 
> > Signed-off-by: Sage Weil <sage@newdream.net>
> > ---
> >  fs/btrfs/ioctl.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index a3c4751..f038d4a 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -2320,7 +2320,12 @@ static noinline long btrfs_ioctl_clone(struct
file *file, unsigned long srcfd,
> >  			else
> >  				new_key.offset = destoff;
> >  
> > -			trans = btrfs_start_transaction(root, 1);
> > +			/*
> > +			 * 1 - adjusting old extent (we may have to split it)
> 
> I don''t think it is enough. If we have lots of file extents and
their extent items are stored
> in many contiguous leaves, the drop operation may cause those leaves to be
COWed. So I think we
> must calculate the number of leaves which will be COWed at first.
Hmm, yes, but if that''s the case, most of the other
btrfs_drop_extents()
callers are broken too.  Take btrfs_cont_expand(), which does more or less 
the same thing that we''re doing too but reserves 2 (no inode update).

Josef, correct me if I''m wrong, but I''m guessing how this
works is that we
are reserving space for something along the lines of 2 * tree depth, or 
enough space to cow the leaf blocks and their parents (plus any space for 
splits/merges).  For inserts, we''d need to be careful about how many
items
we insert (more new leaves), but for removals, as long as we are deleting 
sequential items, we need at most two leaves, for the new versions of each 
end of the deleted range.  Is that right?

Hmm, how does the reservation interace with the extent backrefs, though?  
Is space reserved for that?

sage


> 
> Thanks
> Miao
> 
> > +			 * 1 - add new extent
> > +			 * 1 - inode update
> > +			 */
> > +			trans = btrfs_start_transaction(root, 3);
> >  			if (IS_ERR(trans)) {
> >  				ret = PTR_ERR(trans);
> >  				goto out;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html