-
Notifications
You must be signed in to change notification settings - Fork 257
btrfs-progs: offline filesystem resize feature #1007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: devel
Are you sure you want to change the base?
Conversation
Will this allow for resizing a fs that would normally go ro due to enospc during mount? |
Yeah, I don't see why not! |
Unfortunately not that simple. If the fs has metadata full, it may still fail. Thankfully this one is only mostly modifying chunk tree, which is not that common to exhaust system chunks. Please use chunk root instead since you're only modifying chunk tree. Furthermore there are a lot of code style problems, like unexpected new lines splitting variable definitions (e.g. between And the parameter list for that function is also pretty bad. I'd say normally we put more important parameter (like path, which specifies a whole fs) first, and more specific parameters like new size after that. |
And a lot of error handling is missing. E.g. if |
Ahh yeah, that makes sense.
Good catch... I don't have the best understanding of btrfs transactions. When I was testing this patch it appeared like the filesystem was being resized correctly, why would this be the case if I'm passing the wrong target? Was it making changes to the wrong tree? Was I just getting lucky?
Will do.
Will check this in v2.
I'll rethink the parameter ordering. I pass the amount parameter as a char because I call check_offline_resize_args inside offline_resize and I wanted to keep all of the argument logic together. |
Ok, makes sense will fix in v2. |
The target root for
You can check how other codes in btrfs-progs is doing. There are exceptions like the existing In your particular case, we won't support anything other than a number, thus parsing it early will be a more common solution. BTW, for your update, you can just force push the same branch. No need to create a new PR. |
In this case there is a difference between passing "+1G" and "1G" that would be lost if I just passed a u64. "+1G" increases the filesystem size by 1G and "1G" sets the filesystem size to 1G. I could pass a u64 along with a boolean to indicate whether the size is relative to the existing filesystem size, but it feels like this logic would be better encapsulated inside check_offline_resize_args. |
An offline tool that increases a device's size--and does nothing else--is much more likely to succeed in that situation than mount + fi-resize, but it can still fail in one really specific case. mount does a number of things that can result in enospc failure. Off the top of my head: orphan inode reclaim, the snapshot cleaner thread, tearing down an incomplete reloc tree, and resuming a balance (this is the only one that can be turned off by a mount option). Avoiding these other tasks makes success far more likely. The chunk tree is stored in the system chunk, which is a dedicated contiguous storage area that is usually already large enough for two copies of the chunk tree. To run out of space there, you'd need hundreds of thousands of chunks, like a Users can run into this case fairly often when they have filesystems that are close to this threshold size (100 TiB or multiples thereof). A smaller filesystem doesn't allocate enough chunks to require a new system chunk, while a bigger filesystem would have unallocated space when it needs a new system chunk. It also comes up if you're running any striped profile (raid0, raid10, raid5, or raid6) and you don't do something to reduce dev_extent fragmentation--I hit this with a 26T filesystem once, that had 250k chunks after several drive upgrades and naive balances. If you're willing to accept overwriting a metadata page in place, you can resize a device by finding the page in the chunk tree where the device item is, changing the size field in the dev item, and updating the csum (in addition to the superblock changes, which are already overwrite-in-place, and repeated across all mirrors). Increasing or decreasing a device size (without relocating any chunks) doesn't require any new allocations. |
Just use |
d7e49b7
to
44e653a
Compare
Force pushed with a v2
I'm still passing the amount as a cstring because a s64 doesn't convey the difference between "+1G" and "1G". |
/* For target sizes without +/- sign prefix (e.g. 1:150g) */ | ||
if (mod == 0) { | ||
new_size = diff; | ||
} else if (mod > 0) { |
Check warning
Code scanning / CodeQL
Comparison result is always the same Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bot is right here. mod should be a bool, it's only ever 0 or 1
f660864
to
a452b1e
Compare
@adam900710 ping for v2 review |
Use SUBVOL_SYNC_WAIT ioctl for 'btrfs subvolume sync' command before checking periodically and add an option to not use sync wait ioctl call and force to check periodically. This patch calls a new function wait_for_subvolume_sync() that calls BTRFS_IOC_SUBVOL_SYNC_WAIT for each subvol. Issue: kdave#953 Pull-request: kdave#989 Signed-off-by: Sidong Yang <sidong.yang@furiosa.ai> Signed-off-by: David Sterba <dsterba@suse.com>
In the upstream equivalent of btrfs_remove_block_group(), the function remove_block_group_free_space() is called to delete free spaces associated with the block group being freed. However, this function is defined in btrfs-progs but not called anywhere. To address this issue, I added a call to remove_block_group_free_space() in btrfs_remove_block_group(). This ensures that the free spaces are properly deleted when a block group is removed. I also added a check to remove_block_group_free_space to make sure that free-space-tree is enabled. Signed-off-by: Leo Martins <loemra.dev@gmail.com>
check_free_space_tree() is intended to check that the free-space tree doesn't contain any entries which don't correspond to a block group. Unfortunately it calls btrfs_lookup_first_block_group(), which only returns NULL if the search offset is larger than the start of any block group. Fix check_free_space_tree() to use btrfs_lookup_block_group() as was obviously intended, and fix fsck-tests/054-orphan-directory to get rid of its spurious free-space entries that this now diagnoses. Signed-off-by: Mark Harmstone <mark@harmstone.com>
There is a note about a bug that mount a fs RO first, then remount it RW, will make btrfs to skip the orphan roots cleanup. However it's no longer the case after kernel commit 44c0ca211a4d ("btrfs: lift read-write mount setup from mount and remount"), as that commit unify the pre-RW mount checks, and will always do the orphan roots cleanup. Just update the note so that it won't cause any confusion. Reviewed-by: Boris Burkov <boris@bur.io> Signed-off-by: Qu Wenruo <wqu@suse.com>
[BEHAVIOR CHANGE] In the incoming v6.17 kernel release, due to the changes in commit 40426dd147ff ("btrfs: use the super_block as holder when mounting file systems"), we can no longer mount a seed device through both sprouted fs and the seed device. E.g. # mkfs.btrfs -f /dev/test/scratch1 # mount /dev/test/scratch1 /mnt/btrfs # xfs_io -f -c "pwrite 0 16m" /mnt/btrfs/foobar # umount /mnt/btrfs # btrfstune -S1 /dev/test/scratch1 # mount /dev/test/scratch1 /mnt/btrfs # btrfs device add /dev/test/scratch2 /mnt/btrfs Now the sprouted fs is mount, but if one wants to mount the seed device, it will fail: # mount /dev/test/scratch1 /mnt/btrfs/ mount: /mnt/btrfs: /dev/mapper/test-scratch1 already mounted or mount point busy. dmesg(1) may have more information after failed mount system call. The only new dmesg is: BTRFS error: failed to open device for path /dev/mapper/test-scratch1 with flags 0x23: -16 [CAUSE] After that kernel commit, each block device will have its own unique holder (super block). This super block block device holder is critical to pass device events like freeze/thaw/missing to each filesystem. And after the seed device is sprouted, the holder for the seed device is the super block of the sprouted fs. But if some one else tries to mount the seed device again, it will be a new super block as the holder passed into bdev_file_open_by_path(). Since the seed device already has a different holder, this new bdev_file_open_by_path() will fail with -EBUSY. [ENHANCEMENT] This is a kernel behavior change, but considering the benefit (proper bdev events passing into the fs) I'd say the old behavior is more like a hack or a coincidence, other than a properly designed behavior. So update the documentation to make it more explicit. Reviewed-by: Boris Burkov <boris@bur.io> Signed-off-by: Qu Wenruo <wqu@suse.com>
…erent filesystems Since kernel commit 40426dd147ff ("btrfs: use the super_block as holder when mounting file systems"), the kernel will not allow a block device belonging to two different filesystems. This means a seed device can only be mounted through either the sprouted fs, or the seed device, not both at the same time. Although a seed device can still be shared between different sprouted fs, only one of those fs can be mounted. Considering the extra benefit (extra protection, better device events handling), it's worthy to do the kernel behavior change. Update the test case to follow the new limits. Reviewed-by: Boris Burkov <boris@bur.io> Signed-off-by: Qu Wenruo <wqu@suse.com>
Currently we do the validation of subvolume parameters inside mkfs/main.c, but considering all things like structure rootdir_subvol are all inside rootdir.[ch], it's better to move the validation part into rootdir.[ch]. Extract the validation part into a helper, btrfs_mkfs_validate_subvols(), into rootdir.[ch]. Furthermore since we're here, also slightly enhance the duplicated subvolume check, so that the runtime is halved. Signed-off-by: Qu Wenruo <wqu@suse.com>
helper Currently we do the validation of inode flag parameters inside mkfs/main.c, but considering all things like structure rootdir_inode_flags_entry are all inside rootdir.[ch], it's better to move the validation part into rootdir.[ch]. Extract the validation part into a helper, btrfs_mkfs_validate_inode_flags(), into rootdir.[ch]. Signed-off-by: Qu Wenruo <wqu@suse.com>
Currently for --subvol parameter, we save the full path of the host fs into rootdir_subvol sturcture, then compare each inode we hit with that saved full path. This string comparison can be time consuming (up to PATH_MAX characters), and we're doing it for every directory inode. On the other hand, nftw() also provides a stat structure of the current inode, stat::st_dev and stat::st_ino pair can always uniquely locate an inode in the host filesystem. With that said, we can just save the st_dev/st_ino pair when validating the subvol parameters, and use st_dev/st_ino to check if we hit the target subvolume. This has two benefits: - Reduce the memory usage of each rootdir_subvol Now we need no full_path member, replacing it with two u64. This saves (4K - 16) bytes per rootdir_subvol. - Reduce the runtime of direct inode comparison Instead of doing strcmp() for up to 4K bytes, it's just two u64 comparison. Signed-off-by: Qu Wenruo <wqu@suse.com>
Currently for --inode-flags parameter, we save the full path of the host fs into rootdir_inode_flags_entry sturcture, then compare each inode we hit with that saved full path. This string comparison can be time consuming (up to PATH_MAX characters), and we're doing it for every inode. On the other hand, nftw() also provides a stat structure of the current inode, stat::st_dev and stat::st_ino pair can always uniquely locate an inode in the host filesystem. With that said, we can just save the st_dev/st_ino pair when validating the inode flag parameters, and use st_dev/st_ino to check if we hit the target inode. This has two benefits: - Reduce the memory usage of each rootdir_inode_flags_entry Now we need no full_path member, replacing it with two u64. This saves (4K - 16) bytes per rootdir_inode_flags_entry. - Reduce the runtime of inode comparison Instead of doing strcmp() for up to 4K bytes, it's just two u64 comparison. Signed-off-by: Qu Wenruo <wqu@suse.com>
With the recent enhancements to --subvol and --inode-flags options, the memory usage for them are just 4K+, and even for the older 8K+ memory usage, it's really hard to consider them as heavy memory usage. E.g. even if the end user specified over 1000 entries of --subvol/--inode-flags, it only takes a little over 8MiB for the older code or over 4MiB for the newer code. Since we're in the user space and the year is 2025 not 1995, such memory usage is far from heavy. Just remove the paranoid note from the man page. Signed-off-by: Qu Wenruo <wqu@suse.com>
b62465f
to
f4799f1
Compare
This patch introduces the ability to resize a btrfs filesystem while it is not mounted via a new `--offline` flag. Currently only increasing the size of the filesystem is supported, though I believe it would be possible to implement shrinking the filesystem to the end of the last device extent. This is a more general, and hopefully more useful, solution to the problem I was trying to solve with the ("btrfs-progs: add slack space for mkfs --shrink") patch. This patch should enable users to resize a filesystem without the higher capabilities needed for mounting a filesystem. Signed-off-by: Leo Martins <loemra.dev@gmail.com> --- Changelog: v1->v2: - use chunk root instead of fs root - fix offline resize error handling - fix variable declarations to not have newlines - fix parameter list to have more important arguments first
f4799f1
to
7a8c4e5
Compare
return 0; | ||
} | ||
|
||
static int offline_resize(const char *path, const char *amount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just return bool, if you don't particularly care about the actual return value? It'd simplify some of the code.
(bool is a relatively recent addition to C, so there's still a historic tendency to use int for the same thing in some places)
|
||
root = open_ctree(path, 0, OPEN_CTREE_WRITES | OPEN_CTREE_CHUNK_ROOT_ONLY); | ||
if (!root) { | ||
error("could not open file at %s\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might not even need to print an error here, it looks like open_ctree is already quite chatty. But if you did keep it I think you need to make it clear it works with block devices as well as files
goto close; | ||
} | ||
|
||
ret = snprintf(dev_name, BTRFS_VOL_NAME_MAX, "%s", device->name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sprintf with %s is a code smell - that's just a more complicated strcpy. strncpy is the particular function you're looking for here.
But I'd just move it off the stack and use strdup, that way you don't have to worry about it being too long. Don't forget to free it later.
btrfs_abort_transaction(trans, ret); | ||
goto close; | ||
} | ||
ret |= btrfs_commit_transaction(trans, root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for |=, ret is guaranteed to be 0 here
|
||
if (path_is_reg_file(dev_name)) | ||
ret = truncate(dev_name, new_size); | ||
if (ret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move this check into the scope of the if on line 1520
|
||
if (devstr && !dev_found) { | ||
/* Devid specified but not found. */ | ||
error("cannot find devid: %lld", devid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devid is unsigned, this should be %llu. The -Wformat-signedness warning will diagnose this
new_size = 0; | ||
} else { | ||
new_size = stat_buf.st_size; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit-picking, but the usual kernel style is that you wouldn't need curly brackets here
/* For target sizes without +/- sign prefix (e.g. 1:150g) */ | ||
if (mod == 0) { | ||
new_size = diff; | ||
} else if (mod > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bot is right here. mod should be a bool, it's only ever 0 or 1
|
||
if (new_size < 256 * SZ_1M) | ||
warning("the new size %lld (%s) is < 256MiB, this may be rejected by kernel", | ||
new_size, pretty_size_mode(new_size, UNITS_DEFAULT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%llu
warning("the new size %lld (%s) is < 256MiB, this may be rejected by kernel", | ||
new_size, pretty_size_mode(new_size, UNITS_DEFAULT)); | ||
|
||
pr_verbose(LOG_DEFAULT, "Resize device id %lld from %s to %s\n", devid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%llu
05e416f
to
539b0cb
Compare
This patch introduces the ability to resize a btrfs filesystem while it is not mounted via a new
--offline
flag. Currently only increasing the size of the filesystem is supported, though I believe it would be possible to implement shrinking the filesystem to the end of the last device extent.This is a more general, and hopefully more useful, solution to the problem I was trying to solve with the
("btrfs-progs: add slack space for mkfs --shrink") patch. This patch should enable users to resize a filesystem without the higher capabilities needed for mounting a filesystem.