Re: [PATCH v2 2/2] Btrfs: compression must free at least PAGE_SIZE

[Duncan]

Timofey Titovets posted on Sat, 20 May 2017 21:30:47 +0300 as excerpted:

> 2017-05-20 20:14 GMT+03:00 Kai Krakow :
> 
>> BTW: What's the smallest block size that btrfs stores? Is it always
>> PAGE_SIZE? I'm not familiar with btrfs internals...

Thanks for asking the question. =:^)  I hadn't made the below conflicting 
patch sets association until I saw it.

> https://btrfs.wiki.kernel.org/index.php/Manpage/mkfs.btrfs

> AFAIK btrfs works with storage and account data by PAGE_SIZEd block,
> so it's must be usefull to check if compressed size will give as at
> least one PAGE_SIZE space.

[Not a dev, just a btrfs list regular and btrfs user.  If the devs say 
different...]

While AFAIK, btrfs now saves data in page-size blocks (tho note that if 
the entire file is under a block it may be inlined into metadata 
depending on various limits, at least one of which is configurable)...

There is a sub-page-size patch set that has already been thru several 
revision cycles and AFAIK, remains on the roadmap for eventual merge.

You'd need to check the list or ask the devs about current status, but 
it's quite possible the current code checking for at least one byte of 
compression savings, instead of the at least PAGE_SIZE bytes of savings 
that at present would make more sense, is in anticipation of that patch 
set being merged.

If the sub-page-size block patch-set is no longer anticipated to be 
merged in the relatively near future, it may be worth changing the 
compression profit checks to PAGE_SIZE minimum, as this patch set does, 
but the two patch sets clearly do conflict in concept, so merging this 
one is unlikely to be wise if the other one is still on track for merging.

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

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

Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Sargun Dhillon]

Just as a follow-up, folks are working around this in userspace:
https://github.com/moby/moby/pull/29427


On Sat, May 20, 2017 at 1:39 AM, Sargun Dhillon  wrote:
> This change follows the change to automatically remove qgroups
> if the associated subvolume has also been removed. It changes
> the default behaviour to automatically remove qgroups when
> a subvolume is deleted, but this can be override with the
> qgroup_keep mount flag.
>
> Signed-off-by: Sargun Dhillon 
> ---
>  fs/btrfs/ctree.h |  1 +
>  fs/btrfs/ioctl.c | 14 ++
>  fs/btrfs/super.c | 16 +++-
>  3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 643c70d..4d57eb6 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct 
> btrfs_fs_info *info)
>  #define BTRFS_MOUNT_FRAGMENT_METADATA  (1 << 25)
>  #define BTRFS_MOUNT_FREE_SPACE_TREE(1 << 26)
>  #define BTRFS_MOUNT_NOLOGREPLAY(1 << 27)
> +#define BTRFS_MOUNT_QGROUP_KEEP(1 << 28)
>
>  #define BTRFS_DEFAULT_COMMIT_INTERVAL  (30)
>  #define BTRFS_DEFAULT_MAX_INLINE   (2048)
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index e176375..b10d7bb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2544,6 +2544,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct 
> file *file,
> }
> }
>
> +   /*
> +* Attempt to automatically remove the automatically attached qgroup
> +* setup in btrfs_qgroup_inherit. As a matter of convention, the id
> +* is the same as the subvolume id.
> +*
> +* This can fail non-fatally for level 0 qgroups, and potentially
> +* leave the filesystem in an awkward, (but working) state.
> +*/
> +   if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
> +   ret = btrfs_remove_qgroup(trans, fs_info,
> + dest->root_key.objectid);
> +   if (ret && ret != -ENOENT)
> +   pr_info("Could not automatically delete qgroup: 
> %d\n", ret);
> +   }
>  out_end_trans:
> trans->block_rsv = NULL;
> trans->bytes_reserved = 0;
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 4f1cdd5..232e1b8 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -321,7 +321,7 @@ enum {
> Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
> Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
> Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
> -   Opt_nologreplay, Opt_norecovery,
> +   Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep, Opt_qgroup_nokeep,
>  #ifdef CONFIG_BTRFS_DEBUG
> Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
>  #endif
> @@ -381,6 +381,8 @@ static const match_table_t tokens = {
> {Opt_rescan_uuid_tree, "rescan_uuid_tree"},
> {Opt_fatal_errors, "fatal_errors=%s"},
> {Opt_commit_interval, "commit=%d"},
> +   {Opt_qgroup_keep, "qgroup_keep"},
> +   {Opt_qgroup_nokeep, "qgroup_nokeep"},
>  #ifdef CONFIG_BTRFS_DEBUG
> {Opt_fragment_data, "fragment=data"},
> {Opt_fragment_metadata, "fragment=metadata"},
> @@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char 
> *options,
> info->commit_interval = 
> BTRFS_DEFAULT_COMMIT_INTERVAL;
> }
> break;
> +   case Opt_qgroup_keep:
> +   btrfs_set_and_info(info, QGROUP_KEEP,
> +  "do not automatically delete 
> qgroups");
> +   break;
> +   case Opt_qgroup_nokeep:
> +   btrfs_clear_and_info(info, QGROUP_KEEP,
> +"automatically delete qgroups");
> +   break;
>  #ifdef CONFIG_BTRFS_DEBUG
> case Opt_fragment_all:
> btrfs_info(info, "fragmenting all space");
> @@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file *seq, 
> struct dentry *dentry)
> seq_puts(seq, ",fatal_errors=panic");
> if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
> seq_printf(seq, ",commit=%d", info->commit_interval);
> +   if (btrfs_test_opt(info, QGROUP_KEEP))
> +   seq_puts(seq, ",qgroup_keep");
> +   else
> +   seq_puts(seq, ",qgroup_nokeep");
>  #ifdef CONFIG_BTRFS_DEBUG
> if (btrfs_test_opt(info, FRAGMENT_DATA))
> seq_puts(seq, ",fragment=data");
> --
> 2.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] Btrfs: compression must free at least PAGE_SIZE

[Timofey Titovets]

Btrfs already skip store of data where compression didn't free at least one 
byte.
So make logic better and make check that compression free at least one 
PAGE_SIZE,
because in another case it useless to store this data compressed

Signed-off-by: Timofey Titovets 
---
 fs/btrfs/lzo.c  | 5 -
 fs/btrfs/zlib.c | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index bd0b0938..39678499 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -229,8 +229,11 @@ static int lzo_compress_pages(struct list_head *ws,
in_len = min(bytes_left, PAGE_SIZE);
}

-   if (tot_out > tot_in)
+   /* Compression must save at least one PAGE_SIZE */
+   if (tot_out + PAGE_SIZE > tot_in) {
+   ret = -E2BIG;
goto out;
+   }

/* store the size of all chunks of compressed data */
cpage_out = kmap(pages[0]);
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 135b1082..11e117b5 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -191,7 +191,8 @@ static int zlib_compress_pages(struct list_head *ws,
goto out;
}

-   if (workspace->strm.total_out >= workspace->strm.total_in) {
+   /* Compression must save at least one PAGE_SIZE */
+   if (workspace->strm.total_out + PAGE_SIZE > workspace->strm.total_in) {
ret = -E2BIG;
goto out;
}
--
2.13.0
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/2] Btrfs: compression fixes

[Timofey Titovets]

First patch: fix copy paste typo in debug message in lzo.c, lzo is not deflate

Second patch: force btrfs to not store data as compressed, if compression will 
not
free at least one PAGE_SIZE, because it's useless in term of storage and 
reading data from disk, as a result
productivity suffers

Changes since v1:
- Merge patches for zlib and lzo in one
- Sync check logic for zlib and lzo
- Check profit after all data are compressed (not while compressing)

Changes since v2:
- Fix comparassion logic, it's enough if:
  compressed size + PAGE_SIZE not bigger then input data size

Timofey Titovets (2):
  Btrfs: lzo.c pr_debug() deflate->lzo
  Btrfs: compression must free at least PAGE_SIZE

 fs/btrfs/lzo.c  | 7 +--
 fs/btrfs/zlib.c | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

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

[PATCH v3 1/2] Btrfs: lzo.c pr_debug() deflate->lzo

[Timofey Titovets]

Signed-off-by: Timofey Titovets 
---
 fs/btrfs/lzo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index f48c8c14..bd0b0938 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -141,7 +141,7 @@ static int lzo_compress_pages(struct list_head *ws,
ret = lzo1x_1_compress(data_in, in_len, workspace->cbuf,
   _len, workspace->mem);
if (ret != LZO_E_OK) {
-   pr_debug("BTRFS: deflate in loop returned %d\n",
+   pr_debug("BTRFS: lzo in loop returned %d\n",
   ret);
ret = -EIO;
goto out;
--
2.13.0
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] Btrfs: compression must free at least PAGE_SIZE

[Timofey Titovets]

2017-05-20 20:14 GMT+03:00 Kai Krakow :
> Am Sat, 20 May 2017 19:49:53 +0300
> schrieb Timofey Titovets :
>
>> Btrfs already skip store of data where compression didn't free at
>> least one byte. So make logic better and make check that compression
>> free at least one PAGE_SIZE, because in another case it useless to
>> store this data compressed
>>
>> Signed-off-by: Timofey Titovets 
>> ---
>>  fs/btrfs/lzo.c  | 5 -
>>  fs/btrfs/zlib.c | 3 ++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
>> index bd0b0938..7f38bc3c 100644
>> --- a/fs/btrfs/lzo.c
>> +++ b/fs/btrfs/lzo.c
>> @@ -229,8 +229,11 @@ static int lzo_compress_pages(struct list_head
>> *ws, in_len = min(bytes_left, PAGE_SIZE);
>>   }
>>
>> - if (tot_out > tot_in)
>> + /* Compression must save at least one PAGE_SIZE */
>> + if (tot_out + PAGE_SIZE => tot_in) {
>
> Shouldn't this be ">" instead of ">=" (btw, I don't think => works)...
(D'oh, typo, thanks)

> Given the case that tot_in is 8192, and tot_out is 4096, we saved a
> complete page but 4096+4096 would still be equal to 8192.
>
> The former logic only pretended that there is no point in compression
> if we saved 0 bytes.

Before my changes,
lzo: if compressed data use same or less space in compare to not
compressed -> save compressed version
zlib: if profit of compression will not save at least one byte -> not
compress that data

zlib logic is right, so i just did copy-paste that, but after my
changes this case where compressed size == input size doesn't valid
(i.e. it's ok then tot_out + PAGE_SIZE == tot_in),
so you right ">=" -> ">"

i will fix that and resend patch set, thanks.

> BTW: What's the smallest block size that btrfs stores? Is it always
> PAGE_SIZE? I'm not familiar with btrfs internals...
https://btrfs.wiki.kernel.org/index.php/Manpage/mkfs.btrfs
AFAIK btrfs works with storage and account data by PAGE_SIZEd block,
so it's must be usefull to check if compressed size will give as at
least one PAGE_SIZE space.

>
>> + ret = -E2BIG;
>>   goto out;
>> + }
>>
>>   /* store the size of all chunks of compressed data */
>>   cpage_out = kmap(pages[0]);
>> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
>> index 135b1082..2b04259b 100644
>> --- a/fs/btrfs/zlib.c
>> +++ b/fs/btrfs/zlib.c
>> @@ -191,7 +191,8 @@ static int zlib_compress_pages(struct list_head
>> *ws, goto out;
>>   }
>>
>> - if (workspace->strm.total_out >= workspace->strm.total_in) {
>> + /* Compression must save at least one PAGE_SIZE */
>> + if (workspace->strm.total_out + PAGE_SIZE >=
>> workspace->strm.total_in) { ret = -E2BIG;
>
> Same as above...
>
>>   goto out;
>>   }
>> --
>> 2.13.0
>
>
> --
> Regards,
> Kai



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

Re: [PATCH v2 2/2] Btrfs: compression must free at least PAGE_SIZE

[Kai Krakow]

Am Sat, 20 May 2017 19:49:53 +0300
schrieb Timofey Titovets :

> Btrfs already skip store of data where compression didn't free at
> least one byte. So make logic better and make check that compression
> free at least one PAGE_SIZE, because in another case it useless to
> store this data compressed
> 
> Signed-off-by: Timofey Titovets 
> ---
>  fs/btrfs/lzo.c  | 5 -
>  fs/btrfs/zlib.c | 3 ++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
> index bd0b0938..7f38bc3c 100644
> --- a/fs/btrfs/lzo.c
> +++ b/fs/btrfs/lzo.c
> @@ -229,8 +229,11 @@ static int lzo_compress_pages(struct list_head
> *ws, in_len = min(bytes_left, PAGE_SIZE);
>   }
> 
> - if (tot_out > tot_in)
> + /* Compression must save at least one PAGE_SIZE */
> + if (tot_out + PAGE_SIZE => tot_in) {

Shouldn't this be ">" instead of ">=" (btw, I don't think => works)...

Given the case that tot_in is 8192, and tot_out is 4096, we saved a
complete page but 4096+4096 would still be equal to 8192.

The former logic only pretended that there is no point in compression
if we saved 0 bytes.

BTW: What's the smallest block size that btrfs stores? Is it always
PAGE_SIZE? I'm not familiar with btrfs internals...

> + ret = -E2BIG;
>   goto out;
> + }
> 
>   /* store the size of all chunks of compressed data */
>   cpage_out = kmap(pages[0]);
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index 135b1082..2b04259b 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -191,7 +191,8 @@ static int zlib_compress_pages(struct list_head
> *ws, goto out;
>   }
> 
> - if (workspace->strm.total_out >= workspace->strm.total_in) {
> + /* Compression must save at least one PAGE_SIZE */
> + if (workspace->strm.total_out + PAGE_SIZE >=
> workspace->strm.total_in) { ret = -E2BIG;

Same as above...

>   goto out;
>   }
> --
> 2.13.0


-- 
Regards,
Kai

Replies to list-only preferred.

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

[PATCH v2 0/2] Btrfs: compression fixes

[Timofey Titovets]

First patch: fix copy paste typo in debug message in lzo.c, lzo is not deflate

Second patch: force btrfs to not store data as compressed, if compression will 
not
free at least one PAGE_SIZE, because it's useless in term of storage and 
reading data from disk, as a result
productivity suffers

Changes since v1:
- Merge patches for zlib and lzo in one
- Sync check logic for zlib and lzo
- Check profit after all data are compressed (not while compressing)

Timofey Titovets (2):
  Btrfs: lzo.c pr_debug() deflate->lzo
  Btrfs: compression must free at least PAGE_SIZE

 fs/btrfs/lzo.c  | 7 +--
 fs/btrfs/zlib.c | 3 ++-
 2 files changed, 7 insertions(+), 3 deletions(-)

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

[PATCH v2 2/2] Btrfs: compression must free at least PAGE_SIZE

[Timofey Titovets]

Btrfs already skip store of data where compression didn't free at least one 
byte.
So make logic better and make check that compression free at least one 
PAGE_SIZE,
because in another case it useless to store this data compressed

Signed-off-by: Timofey Titovets 
---
 fs/btrfs/lzo.c  | 5 -
 fs/btrfs/zlib.c | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index bd0b0938..7f38bc3c 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -229,8 +229,11 @@ static int lzo_compress_pages(struct list_head *ws,
in_len = min(bytes_left, PAGE_SIZE);
}

-   if (tot_out > tot_in)
+   /* Compression must save at least one PAGE_SIZE */
+   if (tot_out + PAGE_SIZE => tot_in) {
+   ret = -E2BIG;
goto out;
+   }

/* store the size of all chunks of compressed data */
cpage_out = kmap(pages[0]);
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 135b1082..2b04259b 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -191,7 +191,8 @@ static int zlib_compress_pages(struct list_head *ws,
goto out;
}

-   if (workspace->strm.total_out >= workspace->strm.total_in) {
+   /* Compression must save at least one PAGE_SIZE */
+   if (workspace->strm.total_out + PAGE_SIZE >= workspace->strm.total_in) {
ret = -E2BIG;
goto out;
}
--
2.13.0
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] Btrfs: lzo.c pr_debug() deflate->lzo

[Timofey Titovets]

Signed-off-by: Timofey Titovets 
---
 fs/btrfs/lzo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index f48c8c14..bd0b0938 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -141,7 +141,7 @@ static int lzo_compress_pages(struct list_head *ws,
ret = lzo1x_1_compress(data_in, in_len, workspace->cbuf,
   _len, workspace->mem);
if (ret != LZO_E_OK) {
-   pr_debug("BTRFS: deflate in loop returned %d\n",
+   pr_debug("BTRFS: lzo in loop returned %d\n",
   ret);
ret = -EIO;
goto out;
--
2.13.0
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] Btrfs: lzo compression must free at least PAGE_SIZE

[Timofey Titovets]

2017-05-20 18:47 GMT+03:00 Lionel Bouton :
> Le 19/05/2017 à 23:15, Timofey Titovets a écrit :
>> 2017-05-19 23:19 GMT+03:00 Lionel Bouton
>> :
>>> I was too focused on other problems and having a fresh look at what I
>>> wrote I'm embarrassed by what I read. Used pages for a given amount
>>> of data should be (amount / PAGE_SIZE) + ((amount % PAGE_SIZE) == 0 ?
>>> 0 : 1) this seems enough of a common thing to compute that the kernel
>>> might have a macro defined for this.
>> If i understand the code correctly, the logic of comparing the size of
>> input/output by bytes is enough (IMHO)
>
> As I suspected I missed context : the name of the function makes it
> clear it is supposed to work on whole pages so you are right about the
> comparison.
>
> What I'm still unsure is if the test is at the right spot. The inner
> loop seems to work on at most
> in_len = min(len, PAGE_SIZE)
> chunks of data,
> for example on anything with len >= 4xPAGE_SIZE and PAGE_SIZE=4096 it
> seems to me there's a problem.
>
> if·(tot_in·>·8192·&&·tot_in·<·tot_out·+·PAGE_SIZE)
>
> tot_in > 8192 is true starting at the 3rd page being processedin my example
>
> If the 3 first pages don't manage to free one full page (ie the function
> only reaches at best a 2/3 compression ratio) the modified second
> condition is true and the compression is aborted. This even if
> continuing the compression would end up in freeing one page (tot_out is
> expected to rise slower than tot_in on compressible data so the
> difference could rise and reach a full PAGE_SIZE).
>
> Am I still confused by something ?
>
> Best regards,
>
> Lionel

You right, size must be checked after all data are already comressed,
so i will fix that and update patch set, thanks

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

Re: [PATCH 2/3] Btrfs: lzo compression must free at least PAGE_SIZE

[Lionel Bouton]

Le 19/05/2017 à 23:15, Timofey Titovets a écrit :
> 2017-05-19 23:19 GMT+03:00 Lionel Bouton
> :
>> I was too focused on other problems and having a fresh look at what I
>> wrote I'm embarrassed by what I read. Used pages for a given amount
>> of data should be (amount / PAGE_SIZE) + ((amount % PAGE_SIZE) == 0 ?
>> 0 : 1) this seems enough of a common thing to compute that the kernel
>> might have a macro defined for this. 
> If i understand the code correctly, the logic of comparing the size of
> input/output by bytes is enough (IMHO)

As I suspected I missed context : the name of the function makes it
clear it is supposed to work on whole pages so you are right about the
comparison.

What I'm still unsure is if the test is at the right spot. The inner
loop seems to work on at most
in_len = min(len, PAGE_SIZE)
chunks of data,
for example on anything with len >= 4xPAGE_SIZE and PAGE_SIZE=4096 it
seems to me there's a problem.

if·(tot_in·>·8192·&&·tot_in·<·tot_out·+·PAGE_SIZE)

tot_in > 8192 is true starting at the 3rd page being processedin my example

If the 3 first pages don't manage to free one full page (ie the function
only reaches at best a 2/3 compression ratio) the modified second
condition is true and the compression is aborted. This even if
continuing the compression would end up in freeing one page (tot_out is
expected to rise slower than tot_in on compressible data so the
difference could rise and reach a full PAGE_SIZE).

Am I still confused by something ?

Best regards,

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

Re: Can't remount a BTRFS partition read write after a drive failure

[Sylvain Leroux]

On 05/19/2017 04:23 AM, Duncan wrote:
> > What you seem to have missed is my earlier reply, saying,
> effectively...
> > Known issue. [...]
Indeed I missed that reply. Sorry about that.

Thank you Duncan for your reply, for the great explanations AND for
having taken the time to repost it!


> > So try again in a few kernel cycles...

We will do exactly as you say.

-- 
-- Sylvain Leroux
-- sylv...@chicoree.fr
-- http://www.chicoree.fr



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

[PATCH 8/8] btrfs: Add new ioctl uapis for qgroup creation / removal

[Sargun Dhillon]

This patch ties together the work in the previous patches, to introduce
semantics around the qgroup creation / removal API that are a bit more
intuitive that the current one. It also creates two new args structures
which have reserved space for future expansion, as opposed to single
datastructure for both creation and removal.

The associated semantics are as follows:
1) You cannot create a level 0 qgroup for a subvol which does not exist
   unless you pass in an override flag.
2) You cannot delete a level 0 qgroup which refers to a subvolume, which
   currently exists unless you pass in an override flag.

Signed-off-by: Sargun Dhillon 
---
 fs/btrfs/ioctl.c   | 98 ++
 include/uapi/linux/btrfs.h | 23 +++
 2 files changed, 121 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 5e20643..7bf34b7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4936,6 +4936,100 @@ static long btrfs_ioctl_qgroup_assign(struct file 
*file, void __user *arg)
return ret;
 }
 
+static long btrfs_ioctl_qgroup_create_v2(struct file *file, void __user *arg)
+{
+   struct btrfs_ioctl_qgroup_create_args_v2 create_args;
+   struct inode *inode = file_inode(file);
+   struct btrfs_trans_handle *trans;
+   struct btrfs_fs_info *fs_info;
+   struct btrfs_root *root;
+   int check_subvol_exists;
+   int ret, err;
+
+   fs_info = btrfs_sb(inode->i_sb);
+   root = BTRFS_I(inode)->root;
+
+   if (!capable(CAP_SYS_ADMIN))
+   return -EPERM;
+
+   ret = copy_from_user(_args, arg, sizeof(create_args));
+   if (ret)
+   return ret;
+
+   if (!create_args.qgroupid)
+   return -EINVAL;
+
+   ret = mnt_want_write_file(file);
+   if (ret)
+   return ret;
+
+   trans = btrfs_join_transaction(root);
+   if (IS_ERR(trans)) {
+   ret = PTR_ERR(trans);
+   goto out;
+   }
+
+   check_subvol_exists = !(create_args.flags &
+   BTRFS_QGROUP_CREATE_IGNORE_UNUSED);
+   ret = btrfs_create_qgroup(trans, fs_info, create_args.qgroupid,
+ check_subvol_exists);
+
+   err = btrfs_end_transaction(trans);
+   if (err && !ret)
+   ret = err;
+
+out:
+   mnt_drop_write_file(file);
+   return ret;
+}
+
+static long btrfs_ioctl_qgroup_remove_v2(struct file *file, void __user *arg)
+{
+   struct btrfs_ioctl_qgroup_remove_args_v2 remove_args;
+   struct inode *inode = file_inode(file);
+   struct btrfs_trans_handle *trans;
+   struct btrfs_fs_info *fs_info;
+   struct btrfs_root *root;
+   int check_in_use;
+   int ret, err;
+
+   fs_info = btrfs_sb(inode->i_sb);
+   root = BTRFS_I(inode)->root;
+
+   if (!capable(CAP_SYS_ADMIN))
+   return -EPERM;
+
+   ret = copy_from_user(_args, arg, sizeof(remove_args));
+   if (ret)
+   return ret;
+
+   if (!remove_args.qgroupid)
+   return -EINVAL;
+
+   ret = mnt_want_write_file(file);
+   if (ret)
+   return ret;
+
+   trans = btrfs_join_transaction(root);
+   if (IS_ERR(trans)) {
+   ret = PTR_ERR(trans);
+   goto out;
+   }
+
+   check_in_use = !(remove_args.flags &
+BTRFS_QGROUP_REMOVE_NO_CHECK_IN_USE);
+   ret = btrfs_remove_qgroup(trans, fs_info, remove_args.qgroupid,
+ check_in_use);
+
+   err = btrfs_end_transaction(trans);
+   if (err && !ret)
+   ret = err;
+
+out:
+   mnt_drop_write_file(file);
+   return ret;
+}
+
 static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
 {
struct inode *inode = file_inode(file);
@@ -5626,6 +5720,10 @@ long btrfs_ioctl(struct file *file, unsigned int
return btrfs_ioctl_qgroup_assign(file, argp);
case BTRFS_IOC_QGROUP_CREATE:
return btrfs_ioctl_qgroup_create(file, argp);
+   case BTRFS_IOC_QGROUP_CREATE_V2:
+   return btrfs_ioctl_qgroup_create_v2(file, argp);
+   case BTRFS_IOC_QGROUP_REMOVE_V2:
+   return btrfs_ioctl_qgroup_remove_v2(file, argp);
case BTRFS_IOC_QGROUP_LIMIT:
return btrfs_ioctl_qgroup_limit(file, argp);
case BTRFS_IOC_QUOTA_RESCAN:
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index a456e53..0a6652d 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -653,6 +653,25 @@ struct btrfs_ioctl_qgroup_create_args {
__u64 create;
__u64 qgroupid;
 };
+
+/* Allow the user to delete qgroups even if there isn't a subvol with the id */
+#defineBTRFS_QGROUP_CREATE_IGNORE_UNUSED   (1ULL << 0)
+
+struct btrfs_ioctl_qgroup_create_args_v2 {
+   __u64 qgroupid;
+   __u64 flags;
+   __u64 

[PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists

[Sargun Dhillon]

This patch is to prepare for following patches in this patchset. The
purpose is to make it so that we can prevent accidental removal of
qgroups that are actively in use.

Signed-off-by: Sargun Dhillon 
---
 fs/btrfs/ioctl.c  |  4 ++--
 fs/btrfs/qgroup.c | 50 +-
 fs/btrfs/qgroup.h |  3 ++-
 3 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b10d7bb..2b1a8c1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2554,7 +2554,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file 
*file,
 */
if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
ret = btrfs_remove_qgroup(trans, fs_info,
- dest->root_key.objectid);
+ dest->root_key.objectid, 0);
if (ret && ret != -ENOENT)
pr_info("Could not automatically delete qgroup: %d\n", 
ret);
}
@@ -4974,7 +4974,7 @@ static long btrfs_ioctl_qgroup_create(struct file *file, 
void __user *arg)
if (sa->create) {
ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid);
} else {
-   ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid);
+   ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid, 0);
}
 
err = btrfs_end_transaction(trans);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 588248b..a0699fd 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1247,6 +1247,45 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle 
*trans,
return ret;
 }
 
+/*
+ * Meant to only operate on level-0 qroupid.
+ *
+ * It returns 1 if a matching subvolume is found; 0 if none is found.
+ * < 0 if there is an error.
+ */
+static int btrfs_subvolume_exists(struct btrfs_fs_info *fs_info, u64 qgroupid)
+{
+   struct btrfs_path *path;
+   struct btrfs_key key;
+   int err, ret = 0;
+
+   path = btrfs_alloc_path();
+   if (!path)
+   return -ENOMEM;
+
+   key.objectid = qgroupid;
+   key.type = BTRFS_ROOT_BACKREF_KEY;
+   key.offset = 0;
+
+   err = btrfs_search_slot_for_read(fs_info->tree_root, , path, 1, 0);
+   if (err == 1)
+   goto out;
+
+   if (err) {
+   ret = err;
+   goto out;
+   }
+
+   btrfs_item_key_to_cpu(path->nodes[0], , path->slots[0]);
+   if (key.objectid != qgroupid || key.type != BTRFS_ROOT_BACKREF_KEY)
+   goto out;
+
+   ret = 1;
+out:
+   btrfs_free_path(path);
+   return ret;
+}
+
 /* Must be called with qgroup_ioctl_lock held */
 static int __btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 struct btrfs_fs_info *fs_info, u64 qgroupid)
@@ -1333,10 +1372,19 @@ static int __btrfs_remove_qgroup(struct 
btrfs_trans_handle *trans,
 }
 
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
-   struct btrfs_fs_info *fs_info, u64 qgroupid)
+   struct btrfs_fs_info *fs_info, u64 qgroupid,
+   int check_in_use)
 {
int ret;
 
+   if (check_in_use && btrfs_qgroup_level(qgroupid) == 0) {
+   ret = btrfs_subvolume_exists(fs_info, qgroupid);
+   if (ret < 0)
+   return ret;
+   if (ret)
+   return -EBUSY;
+   }
+
mutex_lock(_info->qgroup_ioctl_lock);
ret = __btrfs_remove_qgroup(trans, fs_info, qgroupid);
mutex_unlock(_info->qgroup_ioctl_lock);
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index fb6c7da..fc08bdb 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -127,7 +127,8 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle 
*trans,
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 qgroupid);
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
-   struct btrfs_fs_info *fs_info, u64 qgroupid);
+   struct btrfs_fs_info *fs_info, u64 qgroupid,
+   int check_in_use);
 int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
   struct btrfs_fs_info *fs_info, u64 qgroupid,
   struct btrfs_qgroup_limit *limit);
-- 
2.9.3

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

[PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol

[Sargun Dhillon]

This patch is to prepare for following patches in this patchset. The code
allows you to check if a subvol exists, and to only allow the creation
of qgroups on subvols that already exist. It doesn't make sense to allow
the creation of level 0 qgroups otherwise.

The behaviour is to inherit (create) a qgroup when you create a new
subvol with quota on. If there is an existing qgroup with this same
ID, it will be reset.

Signed-off-by: Sargun Dhillon 
---
 fs/btrfs/ioctl.c  |  2 +-
 fs/btrfs/qgroup.c | 11 ++-
 fs/btrfs/qgroup.h |  3 ++-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 2b1a8c1..5e20643 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4972,7 +4972,7 @@ static long btrfs_ioctl_qgroup_create(struct file *file, 
void __user *arg)
 
/* FIXME: check if the IDs really exist */
if (sa->create) {
-   ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid);
+   ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid, 0);
} else {
ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid, 0);
}
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index a0699fd..28e2c7f 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1317,10 +1317,19 @@ static int __btrfs_create_qgroup(struct 
btrfs_trans_handle *trans,
 }
 
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
-   struct btrfs_fs_info *fs_info, u64 qgroupid)
+   struct btrfs_fs_info *fs_info, u64 qgroupid,
+   int check_subvol_exists)
 {
int ret;
 
+   if (check_subvol_exists && btrfs_qgroup_level(qgroupid) == 0) {
+   ret = btrfs_subvolume_exists(fs_info, qgroupid);
+   if (ret < 0)
+   return ret;
+   if (!ret)
+   return -ENOENT;
+   }
+
mutex_lock(_info->qgroup_ioctl_lock);
ret = __btrfs_create_qgroup(trans, fs_info, qgroupid);
mutex_unlock(_info->qgroup_ioctl_lock);
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index fc08bdb..1afbe40 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -125,7 +125,8 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle 
*trans,
 int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
  struct btrfs_fs_info *fs_info, u64 src, u64 dst);
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
-   struct btrfs_fs_info *fs_info, u64 qgroupid);
+   struct btrfs_fs_info *fs_info, u64 qgroupid,
+   int check_subvol_exists);
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 qgroupid,
int check_in_use);
-- 
2.9.3

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

[PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Sargun Dhillon]

This change follows the change to automatically remove qgroups
if the associated subvolume has also been removed. It changes
the default behaviour to automatically remove qgroups when
a subvolume is deleted, but this can be override with the
qgroup_keep mount flag.

Signed-off-by: Sargun Dhillon 
---
 fs/btrfs/ctree.h |  1 +
 fs/btrfs/ioctl.c | 14 ++
 fs/btrfs/super.c | 16 +++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 643c70d..4d57eb6 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct 
btrfs_fs_info *info)
 #define BTRFS_MOUNT_FRAGMENT_METADATA  (1 << 25)
 #define BTRFS_MOUNT_FREE_SPACE_TREE(1 << 26)
 #define BTRFS_MOUNT_NOLOGREPLAY(1 << 27)
+#define BTRFS_MOUNT_QGROUP_KEEP(1 << 28)
 
 #define BTRFS_DEFAULT_COMMIT_INTERVAL  (30)
 #define BTRFS_DEFAULT_MAX_INLINE   (2048)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e176375..b10d7bb 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2544,6 +2544,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct file 
*file,
}
}
 
+   /*
+* Attempt to automatically remove the automatically attached qgroup
+* setup in btrfs_qgroup_inherit. As a matter of convention, the id
+* is the same as the subvolume id.
+*
+* This can fail non-fatally for level 0 qgroups, and potentially
+* leave the filesystem in an awkward, (but working) state.
+*/
+   if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
+   ret = btrfs_remove_qgroup(trans, fs_info,
+ dest->root_key.objectid);
+   if (ret && ret != -ENOENT)
+   pr_info("Could not automatically delete qgroup: %d\n", 
ret);
+   }
 out_end_trans:
trans->block_rsv = NULL;
trans->bytes_reserved = 0;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 4f1cdd5..232e1b8 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -321,7 +321,7 @@ enum {
Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
-   Opt_nologreplay, Opt_norecovery,
+   Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep, Opt_qgroup_nokeep,
 #ifdef CONFIG_BTRFS_DEBUG
Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
 #endif
@@ -381,6 +381,8 @@ static const match_table_t tokens = {
{Opt_rescan_uuid_tree, "rescan_uuid_tree"},
{Opt_fatal_errors, "fatal_errors=%s"},
{Opt_commit_interval, "commit=%d"},
+   {Opt_qgroup_keep, "qgroup_keep"},
+   {Opt_qgroup_nokeep, "qgroup_nokeep"},
 #ifdef CONFIG_BTRFS_DEBUG
{Opt_fragment_data, "fragment=data"},
{Opt_fragment_metadata, "fragment=metadata"},
@@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char 
*options,
info->commit_interval = 
BTRFS_DEFAULT_COMMIT_INTERVAL;
}
break;
+   case Opt_qgroup_keep:
+   btrfs_set_and_info(info, QGROUP_KEEP,
+  "do not automatically delete 
qgroups");
+   break;
+   case Opt_qgroup_nokeep:
+   btrfs_clear_and_info(info, QGROUP_KEEP,
+"automatically delete qgroups");
+   break;
 #ifdef CONFIG_BTRFS_DEBUG
case Opt_fragment_all:
btrfs_info(info, "fragmenting all space");
@@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file *seq, 
struct dentry *dentry)
seq_puts(seq, ",fatal_errors=panic");
if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
seq_printf(seq, ",commit=%d", info->commit_interval);
+   if (btrfs_test_opt(info, QGROUP_KEEP))
+   seq_puts(seq, ",qgroup_keep");
+   else
+   seq_puts(seq, ",qgroup_nokeep");
 #ifdef CONFIG_BTRFS_DEBUG
if (btrfs_test_opt(info, FRAGMENT_DATA))
seq_puts(seq, ",fragment=data");
-- 
2.9.3

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

[PATCH 5/8] btrfs: qgroup.h whitespace change

[Sargun Dhillon]

This patch is just changing whitespace alignment in qgroup.h

Signed-off-by: Sargun Dhillon 
---
 fs/btrfs/qgroup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index fe04d3f..fb6c7da 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -127,7 +127,7 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle 
*trans,
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info, u64 qgroupid);
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
- struct btrfs_fs_info *fs_info, u64 qgroupid);
+   struct btrfs_fs_info *fs_info, u64 qgroupid);
 int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
   struct btrfs_fs_info *fs_info, u64 qgroupid,
   struct btrfs_qgroup_limit *limit);
-- 
2.9.3

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

[PATCH 1/8] btrfs: Split up btrfs_remove_qgroup, no logic changes

[Sargun Dhillon]

This change is purely a style change, so that btrfs_remove_qgroup is
split. There is some whitespace changes, but this shouldn't have any
effect on the logic of the code.

Signed-off-by: Sargun Dhillon 
---
 fs/btrfs/qgroup.c | 49 -
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index deffbeb..a2add44 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1281,40 +1281,35 @@ int btrfs_create_qgroup(struct btrfs_trans_handle 
*trans,
return ret;
 }
 
-int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
-   struct btrfs_fs_info *fs_info, u64 qgroupid)
+/* Must be called with qgroup_ioctl_lock held */
+static int __btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
+struct btrfs_fs_info *fs_info, u64 qgroupid)
 {
+   struct btrfs_qgroup_list *list;
struct btrfs_root *quota_root;
struct btrfs_qgroup *qgroup;
-   struct btrfs_qgroup_list *list;
-   int ret = 0;
+   int ret;
 
-   mutex_lock(_info->qgroup_ioctl_lock);
quota_root = fs_info->quota_root;
-   if (!quota_root) {
-   ret = -EINVAL;
-   goto out;
-   }
+   if (!quota_root)
+   return -EINVAL;
 
qgroup = find_qgroup_rb(fs_info, qgroupid);
-   if (!qgroup) {
-   ret = -ENOENT;
-   goto out;
-   } else {
-   /* check if there are no children of this qgroup */
-   if (!list_empty(>members)) {
-   ret = -EBUSY;
-   goto out;
-   }
-   }
+   if (!qgroup)
+   return -ENOENT;
+
+   /* check if there are no children of this qgroup */
+   if (!list_empty(>members))
+   return -EBUSY;
+
ret = del_qgroup_item(trans, quota_root, qgroupid);
 
while (!list_empty(>groups)) {
list = list_first_entry(>groups,
struct btrfs_qgroup_list, next_group);
ret = __del_qgroup_relation(trans, fs_info,
-  qgroupid,
-  list->group->qgroupid);
+   qgroupid,
+   list->group->qgroupid);
if (ret)
goto out;
}
@@ -1322,8 +1317,20 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
spin_lock(_info->qgroup_lock);
del_qgroup_rb(fs_info, qgroupid);
spin_unlock(_info->qgroup_lock);
+
 out:
+   return ret;
+}
+
+int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
+   struct btrfs_fs_info *fs_info, u64 qgroupid)
+{
+   int ret;
+
+   mutex_lock(_info->qgroup_ioctl_lock);
+   ret = __btrfs_remove_qgroup(trans, fs_info, qgroupid);
mutex_unlock(_info->qgroup_ioctl_lock);
+
return ret;
 }
 
-- 
2.9.3

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

[PATCH 3/8] btrfs: Split up btrfs_create_qgroup, no logic changes

[Sargun Dhillon]

This change is purely a style change, so that btrfs_create_qgroup is
split. There is some whitespace changes, but this shouldn't have any
effect on the logic of the code.

Signed-off-by: Sargun Dhillon 
---
 fs/btrfs/qgroup.c | 32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 7e772ba..588248b 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1247,24 +1247,21 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle 
*trans,
return ret;
 }
 
-int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
-   struct btrfs_fs_info *fs_info, u64 qgroupid)
+/* Must be called with qgroup_ioctl_lock held */
+static int __btrfs_create_qgroup(struct btrfs_trans_handle *trans,
+struct btrfs_fs_info *fs_info, u64 qgroupid)
 {
struct btrfs_root *quota_root;
struct btrfs_qgroup *qgroup;
-   int ret = 0;
+   int ret;
 
-   mutex_lock(_info->qgroup_ioctl_lock);
quota_root = fs_info->quota_root;
-   if (!quota_root) {
-   ret = -EINVAL;
-   goto out;
-   }
+   if (!quota_root)
+   return -EINVAL;
+
qgroup = find_qgroup_rb(fs_info, qgroupid);
-   if (qgroup) {
-   ret = -EEXIST;
-   goto out;
-   }
+   if (qgroup)
+   return -EEXIST;
 
ret = add_qgroup_item(trans, quota_root, qgroupid);
if (ret)
@@ -1277,7 +1274,18 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
if (IS_ERR(qgroup))
ret = PTR_ERR(qgroup);
 out:
+   return ret;
+}
+
+int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
+   struct btrfs_fs_info *fs_info, u64 qgroupid)
+{
+   int ret;
+
+   mutex_lock(_info->qgroup_ioctl_lock);
+   ret = __btrfs_create_qgroup(trans, fs_info, qgroupid);
mutex_unlock(_info->qgroup_ioctl_lock);
+
return ret;
 }
 
-- 
2.9.3

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

[PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails

[Sargun Dhillon]

Previously, we were calling del_qgroup_item, and ignoring the return code
resulting in a potential to have divergent in-memory state without an
error. Perhaps, it makes sense to handle this error code, and put the
filesystem into a read only, or similar state.

This patch only adds reporting of the error.

Signed-off-by: Sargun Dhillon 
---
 fs/btrfs/qgroup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index a2add44..7e772ba 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1303,6 +1303,8 @@ static int __btrfs_remove_qgroup(struct 
btrfs_trans_handle *trans,
return -EBUSY;
 
ret = del_qgroup_item(trans, quota_root, qgroupid);
+   if (ret)
+   goto out;
 
while (!list_empty(>groups)) {
list = list_first_entry(>groups,
-- 
2.9.3

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

[PATCH 0/8] BtrFS: QGroups uapi improvements

[Sargun Dhillon]

This patchset contains some improvements to qgroups. It changes the
semantics around how qgroups are dealt with when subvolumes are
deleted, and it also adds two new ioctls for qgroup deletion
and addition.

The new semantic around qgroup removal is that when the qgroup_nokeep
mount flag is set, it when a subvolume is deleted, the associated
level-0 qgroup will also be removed. This does not trickle up to
high level qgroups.

In addition, it adds two new ioctls for qgroup addition and removal
which have flags to protect against creating qgroups for non-existent
volumes, and in addition flags to prevent the deletion of qgroups
that are associated with volumes.

Sargun Dhillon (8):
  btrfs: Split up btrfs_remove_qgroup, no logic changes
  btrfs: Fail on removing qgroup if del_qgroup_item fails
  btrfs: Split up btrfs_create_qgroup, no logic changes
  btrfs: autoremove qgroup by default, and add a mount flag to override
  btrfs: qgroup.h whitespace change
  btrfs: Add code to check if a qgroup's subvol exists
  btrfs: Add code to prevent qgroup creation for a non-existent subvol
  btrfs: Add new ioctl uapis for qgroup creation / removal

 fs/btrfs/ctree.h   |   1 +
 fs/btrfs/ioctl.c   | 116 -
 fs/btrfs/qgroup.c  | 140 ++---
 fs/btrfs/qgroup.h  |   6 +-
 fs/btrfs/super.c   |  16 +-
 include/uapi/linux/btrfs.h |  23 
 6 files changed, 264 insertions(+), 38 deletions(-)

-- 
2.9.3

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