[review] Btrfs: Allow shrinking close to used space

[review] Btrfs: Allow shrinking close to used space

[Chris Ball]

Hi,

This patch (against experimental HEAD) attempts to make shrinking more
robust, by only updating device size if we've succeeded in creating
enough free space without any failures in btrfs_relocate_chunk().

Here's a log with my patch applied.  The two things to note are that a
near-limit shrink now works, and that a failed shrink (in this case,
trying to shrink to less than the used space) no longer updates the
device size erroneously:

  http://dev.laptop.org/~cjb/btrfs/shrink-log

Please review carefully -- I'm still new to btrfs.  The short version of
the patch is:

* create a success path, as a break out of the while(1) relocating
  (rather than going to the "done" label).
* move the device size updating code into that path
* leave "path->reada = 2;" behind in the entry path, since path is
  used by the searching operation rather than the later resize.

Thanks!

Signed-off-by: Chris Ball <cjb@laptop.org>

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1316139..e2fa072 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1815,30 +1815,8 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 	if (!path)
 		return -ENOMEM;
 
-	trans = btrfs_start_transaction(root, 1);
-	if (!trans) {
-		ret = -ENOMEM;
-		goto done;
-	}
-
 	path->reada = 2;
 
-	lock_chunks(root);
-
-	device->total_bytes = new_size;
-	if (device->writeable)
-		device->fs_devices->total_rw_bytes -= diff;
-	ret = btrfs_update_device(trans, device);
-	if (ret) {
-		unlock_chunks(root);
-		btrfs_end_transaction(trans, root);
-		goto done;
-	}
-	WARN_ON(diff > old_total);
-	btrfs_set_super_total_bytes(super_copy, old_total - diff);
-	unlock_chunks(root);
-	btrfs_end_transaction(trans, root);
-
 	key.objectid = device->devid;
 	key.offset = (u64)-1;
 	key.type = BTRFS_DEV_EXTENT_KEY;
@@ -1867,7 +1845,7 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 		length = btrfs_dev_extent_length(l, dev_extent);
 
 		if (key.offset + length <= new_size)
-			goto done;
+			break;
 
 		chunk_tree = btrfs_dev_extent_chunk_tree(l, dev_extent);
 		chunk_objectid = btrfs_dev_extent_chunk_objectid(l, dev_extent);
@@ -1880,6 +1858,31 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 			goto done;
 	}
 
+	/*
+	 * We've succeeded in freeing up enough space and can now update
+	 * the device's size.
+	 */
+	trans = btrfs_start_transaction(root, 1);
+	if (!trans) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	lock_chunks(root);
+	device->total_bytes = new_size;
+	if (device->writeable)
+		device->fs_devices->total_rw_bytes -= diff;
+	ret = btrfs_update_device(trans, device);
+	if (ret) {
+		unlock_chunks(root);
+		btrfs_end_transaction(trans, root);
+		goto done;
+	}
+	WARN_ON(diff > old_total);
+	btrfs_set_super_total_bytes(super_copy, old_total - diff);
+	unlock_chunks(root);
+	btrfs_end_transaction(trans, root);
+
 done:
 	btrfs_free_path(path);
 	return ret;


-- 
Chris Ball   <cjb@laptop.org>

Re: [review] Btrfs: Allow shrinking close to used space

[Josef Bacik]

On Wed, Feb 25, 2009 at 02:13:10PM -0500, Chris Ball wrote:
> Hi,
> 
> This patch (against experimental HEAD) attempts to make shrinking more
> robust, by only updating device size if we've succeeded in creating
> enough free space without any failures in btrfs_relocate_chunk().
> 
> Here's a log with my patch applied.  The two things to note are that a
> near-limit shrink now works, and that a failed shrink (in this case,
> trying to shrink to less than the used space) no longer updates the
> device size erroneously:
> 
>   http://dev.laptop.org/~cjb/btrfs/shrink-log
> 
> Please review carefully -- I'm still new to btrfs.  The short version of
> the patch is:
> 
> * create a success path, as a break out of the while(1) relocating
>   (rather than going to the "done" label).
> * move the device size updating code into that path
> * leave "path->reada = 2;" behind in the entry path, since path is
>   used by the searching operation rather than the later resize.
> 
> Thanks!
> 
> Signed-off-by: Chris Ball <cjb@laptop.org>
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 1316139..e2fa072 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1815,30 +1815,8 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
>  	if (!path)
>  		return -ENOMEM;
>  
> -	trans = btrfs_start_transaction(root, 1);
> -	if (!trans) {
> -		ret = -ENOMEM;
> -		goto done;
> -	}
> -
>  	path->reada = 2;
>  
> -	lock_chunks(root);
> -
> -	device->total_bytes = new_size;
> -	if (device->writeable)
> -		device->fs_devices->total_rw_bytes -= diff;

So I think you still want to do this part, to keep the allocator from actually
allocating new space in the area we are trying to cull with the shrink, we just
don't want to update the ondisk stuff just yet, so everything else can be moved
to below the loop.

So this
> -	ret = btrfs_update_device(trans, device);
> -	if (ret) {
> -		unlock_chunks(root);
> -		btrfs_end_transaction(trans, root);
> -		goto done;
> -	}
> -	WARN_ON(diff > old_total);
> -	btrfs_set_super_total_bytes(super_copy, old_total - diff);

to here should all be moved below like you have it.  Other than that it looks
good.  Thanks,

Josef

Re: [review] Btrfs: Allow shrinking close to used space

[Yan Zheng]

2009/2/26 Josef Bacik <josef@redhat.com>:
> On Wed, Feb 25, 2009 at 02:13:10PM -0500, Chris Ball wrote:
>> Hi,
>>
>> This patch (against experimental HEAD) attempts to make shrinking mo=
re
>> robust, by only updating device size if we've succeeded in creating
>> enough free space without any failures in btrfs_relocate_chunk().
>>
>> Here's a log with my patch applied. =A0The two things to note are th=
at a
>> near-limit shrink now works, and that a failed shrink (in this case,
>> trying to shrink to less than the used space) no longer updates the
>> device size erroneously:
>>
>> =A0 http://dev.laptop.org/~cjb/btrfs/shrink-log
>>
>> Please review carefully -- I'm still new to btrfs. =A0The short vers=
ion of
>> the patch is:
>>
>> * create a success path, as a break out of the while(1) relocating
>> =A0 (rather than going to the "done" label).
>> * move the device size updating code into that path
>> * leave "path->reada =3D 2;" behind in the entry path, since path is
>> =A0 used by the searching operation rather than the later resize.
>>
>> Thanks!
>>
>> Signed-off-by: Chris Ball <cjb@laptop.org>
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 1316139..e2fa072 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1815,30 +1815,8 @@ int btrfs_shrink_device(struct btrfs_device *=
device, u64 new_size)
>> =A0 =A0 =A0 if (!path)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
>>
>> - =A0 =A0 trans =3D btrfs_start_transaction(root, 1);
>> - =A0 =A0 if (!trans) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENOMEM;
>> - =A0 =A0 =A0 =A0 =A0 =A0 goto done;
>> - =A0 =A0 }
>> -
>> =A0 =A0 =A0 path->reada =3D 2;
>>
>> - =A0 =A0 lock_chunks(root);
>> -
>> - =A0 =A0 device->total_bytes =3D new_size;
>> - =A0 =A0 if (device->writeable)
>> - =A0 =A0 =A0 =A0 =A0 =A0 device->fs_devices->total_rw_bytes -=3D di=
ff;
>
> So I think you still want to do this part, to keep the allocator from=
 actually
> allocating new space in the area we are trying to cull with the shrin=
k, we just
> don't want to update the ondisk stuff just yet, so everything else ca=
n be moved
> to below the loop.
>
> So this
>> - =A0 =A0 ret =3D btrfs_update_device(trans, device);
>> - =A0 =A0 if (ret) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 unlock_chunks(root);
>> - =A0 =A0 =A0 =A0 =A0 =A0 btrfs_end_transaction(trans, root);
>> - =A0 =A0 =A0 =A0 =A0 =A0 goto done;
>> - =A0 =A0 }
>> - =A0 =A0 WARN_ON(diff > old_total);
>> - =A0 =A0 btrfs_set_super_total_bytes(super_copy, old_total - diff);
>
> to here should all be moved below like you have it. =A0Other than tha=
t it looks
> good. =A0Thanks,
>

This isn't working. we don't call btrfs_update_device here, but it can =
be called
in other places. I think we should add a new field in btrfs_device to
reflect the
on disk device size, and update it when shrinking succeeds.

Regards
Yan Zheng
--
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

Re: [review] Btrfs: Allow shrinking close to used space

[Chris Ball]

Hi,

Here's a new patch that incorporates these comments.  We now update
device->fs_devices->total_rw_bytes before the shrink, as Josef suggests,
and create a new field in btrfs_device to store an on-disk size that is
only updated on a successful shrink operation, as requested by Yan.

(Thanks, Yan and Josef, for the patient explanations.)

==
From: Chris Ball <cjb@laptop.org>

Btrfs: When shrinking, only update disk size on success

Previously, we updated a device's size prior to attempting a shrink
operation.  This patch moves the device resizing logic to only happen if
the shrink completes successfully.  In the process, it introduces a new
field to btrfs_device -- disk_total_bytes -- to track the on-disk size.

Signed-off-by: Chris Ball <cjb@laptop.org>
---
 fs/btrfs/volumes.c |   35 ++++++++++++++++++++++++-----------
 fs/btrfs/volumes.h |    3 +++
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 1316139..303b7d6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1433,7 +1433,7 @@ static noinline int btrfs_update_device(struct btrfs_trans_handle *trans,
 	btrfs_set_device_io_align(leaf, dev_item, device->io_align);
 	btrfs_set_device_io_width(leaf, dev_item, device->io_width);
 	btrfs_set_device_sector_size(leaf, dev_item, device->sector_size);
-	btrfs_set_device_total_bytes(leaf, dev_item, device->total_bytes);
+	btrfs_set_device_total_bytes(leaf, dev_item, device->disk_total_bytes);
 	btrfs_set_device_bytes_used(leaf, dev_item, device->bytes_used);
 	btrfs_mark_buffer_dirty(leaf);
 
@@ -1828,14 +1828,6 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 	device->total_bytes = new_size;
 	if (device->writeable)
 		device->fs_devices->total_rw_bytes -= diff;
-	ret = btrfs_update_device(trans, device);
-	if (ret) {
-		unlock_chunks(root);
-		btrfs_end_transaction(trans, root);
-		goto done;
-	}
-	WARN_ON(diff > old_total);
-	btrfs_set_super_total_bytes(super_copy, old_total - diff);
 	unlock_chunks(root);
 	btrfs_end_transaction(trans, root);
 
@@ -1867,7 +1859,7 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 		length = btrfs_dev_extent_length(l, dev_extent);
 
 		if (key.offset + length <= new_size)
-			goto done;
+			break;
 
 		chunk_tree = btrfs_dev_extent_chunk_tree(l, dev_extent);
 		chunk_objectid = btrfs_dev_extent_chunk_objectid(l, dev_extent);
@@ -1880,6 +1872,26 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 			goto done;
 	}
 
+	/* Shrinking succeeded, else we would be at "done". */
+	trans = btrfs_start_transaction(root, 1);
+	if (!trans) {
+		ret = -ENOMEM;
+		goto done;
+	}
+	lock_chunks(root);
+
+	device->disk_total_bytes = new_size;
+	/* Now btrfs_update_device() will change the on-disk size. */
+	ret = btrfs_update_device(trans, device);
+	if (ret) {
+		unlock_chunks(root);
+		btrfs_end_transaction(trans, root);
+		goto done;
+	}
+	WARN_ON(diff > old_total);
+	btrfs_set_super_total_bytes(super_copy, old_total - diff);
+	unlock_chunks(root);
+	btrfs_end_transaction(trans, root);
 done:
 	btrfs_free_path(path);
 	return ret;
@@ -2959,7 +2971,8 @@ static int fill_device_from_item(struct extent_buffer *leaf,
 	unsigned long ptr;
 
 	device->devid = btrfs_device_id(leaf, dev_item);
-	device->total_bytes = btrfs_device_total_bytes(leaf, dev_item);
+	device->disk_total_bytes = btrfs_device_total_bytes(leaf, dev_item);
+	device->total_bytes = device->disk_total_bytes;
 	device->bytes_used = btrfs_device_bytes_used(leaf, dev_item);
 	device->type = btrfs_device_type(leaf, dev_item);
 	device->io_align = btrfs_device_io_align(leaf, dev_item);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 86c44e9..bf1ba75 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -52,6 +52,9 @@ struct btrfs_device {
 	/* size of the device */
 	u64 total_bytes;
 
+	/* size of the disk */
+	u64 disk_total_bytes;
+
 	/* bytes used */
 	u64 bytes_used;
 
-- 
1.6.1.3


- Chris.
-- 
Chris Ball   <cjb@laptop.org>