Will this allow for resizing a fs that would normally go ro due to enospc during mount?

Will this allow for resizing a fs that would normally go ro due to enospc during mount?

Yeah, I don't see why not!

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.
As this is still using regular transaction protection, metadata still need space for COW.

Thankfully this one is only mostly modifying chunk tree, which is not that common to exhaust system chunks.
But your btrfs_start_transaction() is using fs root, which is the wrong target, and can lead to unexpected ENOSPC error.

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 new_size and old_total inside offline_resize()).
IIRC you can use checkpatch.pl from the kernel, as we mostly follow the same code style here.

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.
Furthermore, the amount parameter as char doesn't make much sense, why not just parse the value and directly pass a s64 instead?

And a lot of error handling is missing.

E.g. if btrfs_update_device() failed, you should abort the current transaction other than continue to commit the transaction.
This also gets rid of the unnecessary ret2 variable.

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. As this is still using regular transaction protection, metadata still need space for COW.

Ahh yeah, that makes sense.

Thankfully this one is only mostly modifying chunk tree, which is not that common to exhaust system chunks. But your btrfs_start_transaction() is using fs root, which is the wrong target, and can lead to unexpected ENOSPC error.

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?

Please use chunk root instead since you're only modifying chunk tree.

Will do.

Furthermore there are a lot of code style problems, like unexpected new lines splitting variable definitions (e.g. between new_size and old_total inside offline_resize()). IIRC you can use checkpatch.pl from the kernel, as we mostly follow the same code style here.

Will check this in v2.

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. Furthermore, the amount parameter as char doesn't make much sense, why not just parse the value and directly pass a s64 instead?

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.

And a lot of error handling is missing.

E.g. if btrfs_update_device() failed, you should abort the current transaction other than continue to commit the transaction. This also gets rid of the unnecessary ret2 variable.

Ok, makes sense will fix in v2.

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?

The target root for btrfs_start_transaction() is only for space reservation purpose. For btrfs-progs, it's only checking if we have enough space. So a wrong root here won't cause a problem, until the metadata space is almost exhausted.
In that case, btrfs_start_transaction() will return ENOSPC.

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.

You can check how other codes in btrfs-progs is doing.
For most cases, if we want to pass a size (or a diff of size), we would pass u64 or s64 instead, and do the parsing before calling the final function.

There are exceptions like the existing check_resize_args() where amount can be the string cancel.
But for most cases, the string parsing is done as soon as possible.
E.g. the size variable inside cmd_filesystem_mkswapfile().

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.

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.

You can check how other codes in btrfs-progs is doing. For most cases, if we want to pass a size (or a diff of size), we would pass u64 or s64 instead, and do the parsing before calling the final function.

There are exceptions like the existing check_resize_args() where amount can be the string cancel. But for most cases, the string parsing is done as soon as possible. E.g. the size variable inside cmd_filesystem_mkswapfile().

In your particular case, we won't support anything other than a number, thus parsing it early will be a more common solution.

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.

Will this allow for resizing a fs that would normally go ro due to enospc during mount?

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 single profile 100 TiB filesystem that has no unallocated space at the moment that the chunk tree needs to be enlarged, with a system chunk that still has the original 32MB size from mkfs. On the other hand, while that's a rare case, it would be an especially painful one that you can't fix with this method.

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.

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.

Just use s64

Force pushed with a 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

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.

Just use s64

I'm still passing the amount as a cstring because a s64 doesn't convey the difference between "+1G" and "1G".

@adam900710 ping for v2 review

Just sent out v3!

I now have 3 commits the first refactors how check_resize_args parses the amount string into it's own function to parse_resize_args so it can be used in check_offline_resize_args. The second commit is the actual offline resize logic which does not allow for multi-device resizing and uses the new parse_resize_args. The third is a new test for offline resize.

Just sent out v4 with updated documentation + fixes

Looks good to me, there are some minor coding things that I might fix once this is in devel as they're more stylistic than functional. Regarding the options, we'll probably settle on --offline for the cases where mounted and unmounted command would make sense. The alternative --unmounted is explicit but longer to type and I think offline is used in thisss context without much confusion.

Thank you!

Merged. Thanks.

A few notes from the post-merge review. I've fixed some of that directly, less trivial updates will be probably in separate commits. I've also edited the changelogs so it's closer to the style I'm trying to keep for the whole progs.

• unhandled errors (fstat)

• error messages lacking information about what went wrong, like a size too large? print what is the size and what is the limit

• mismatching types, "int ret = false"

• use explicit "if (strcpy(...) == 0)" instead of "if (!strcpy(...))"

• no "\n" in error() or warning()

• comments are sentence ended with "."

• new includes should be added to the right section in the list

• changelog: try to write more specific statements instead of generic phrases "the code opptimizes the behaviour so that" unless it's really just the only thing the code does

• changelog style tip: provide information that's useful also in the future

• changelog: use consistent subject styles, with prefixes when it's for a command or for tests etc

• tests: write shorter variables, possibly without "_",

• tests: "rm -f -- ..."