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>
--
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 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 -- 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
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 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, >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
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>
--
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