Re: [PATCH] btrfs: tree-checker: use %zu format string for size_t

[Arnd Bergmann]

On Thu, Dec 7, 2017 at 1:32 AM, Qu Wenruo  wrote:
>
>
> On 2017年12月06日 22:18, Arnd Bergmann wrote:
>> The return value of sizeof() is of type size_t, so we must print it
>> using the %z format modifier rather than %l to avoid this warning
>> on some architectures:
>>
>> fs/btrfs/tree-checker.c: In function 'check_dir_item':
>> fs/btrfs/tree-checker.c:273:50: error: format '%lu' expects argument of type 
>> 'long unsigned int', but argument 5 has type 'u32' {aka 'unsigned int'} 
>> [-Werror=format=]
>
> Any idea about which architecture will cause such warning?
> On x86_64 I always fail to get such warning.

I think all 32-bit architectures:

#ifndef __kernel_size_t
#if __BITS_PER_LONG != 64
typedef unsigned int__kernel_size_t;
typedef int __kernel_ssize_t;
typedef int __kernel_ptrdiff_t;
#else
typedef __kernel_ulong_t __kernel_size_t;
typedef __kernel_long_t __kernel_ssize_t;
typedef __kernel_long_t __kernel_ptrdiff_t;
#endif
#endif

  Arnd
--
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] btrfs-progs: Replace usage of list_for_each with list_for_each_entry

[Nikolay Borisov]

There are a couple of places where instead of the more succinct
list_for_each_entry the code uses list_for_each. This results in
slightly more code with no additional benefit as well as no
coherent pattern. This patch makes the code uniform. No functional
changes

Signed-off-by: Nikolay Borisov 
---
 cmds-filesystem.c |  3 +--
 disk-io.c |  4 +---
 utils.c   |  7 +--
 volumes.c | 15 ---
 4 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 7728430f16a1..7c154589a15f 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -182,8 +182,7 @@ static int uuid_search(struct btrfs_fs_devices *fs_devices, 
const char *search)
if (!strncmp(uuidbuf, search, search_len))
return 1;
 
-   list_for_each(cur, &fs_devices->devices) {
-   device = list_entry(cur, struct btrfs_device, dev_list);
+   list_for_each_entry(device, &fs_devices->devices, dev_list) {
if ((device->label && strcmp(device->label, search) == 0) ||
strcmp(device->name, search) == 0)
return 1;
diff --git a/disk-io.c b/disk-io.c
index f5edc4796619..3d8785d5bb37 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1556,7 +1556,6 @@ static int write_dev_supers(struct btrfs_fs_info *fs_info,
 
 int write_all_supers(struct btrfs_fs_info *fs_info)
 {
-   struct list_head *cur;
struct list_head *head = &fs_info->fs_devices->devices;
struct btrfs_device *dev;
struct btrfs_super_block *sb;
@@ -1566,8 +1565,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info)
 
sb = fs_info->super_copy;
dev_item = &sb->dev_item;
-   list_for_each(cur, head) {
-   dev = list_entry(cur, struct btrfs_device, dev_list);
+   list_for_each_entry(dev, head, dev_list) {
if (!dev->writeable)
continue;
 
diff --git a/utils.c b/utils.c
index 6c0d9fc1bebf..65383282b512 100644
--- a/utils.c
+++ b/utils.c
@@ -804,14 +804,9 @@ static int blk_file_in_dev_list(struct btrfs_fs_devices* 
fs_devices,
const char* file)
 {
int ret;
-   struct list_head *head;
-   struct list_head *cur;
struct btrfs_device *device;
 
-   head = &fs_devices->devices;
-   list_for_each(cur, head) {
-   device = list_entry(cur, struct btrfs_device, dev_list);
-
+   list_for_each_entry(device, &fs_devices->devices, dev_list) {
if((ret = is_same_loop_file(device->name, file)))
return ret;
}
diff --git a/volumes.c b/volumes.c
index ce3a540578fd..2e1fb4a46465 100644
--- a/volumes.c
+++ b/volumes.c
@@ -58,10 +58,8 @@ static struct btrfs_device *__find_device(struct list_head 
*head, u64 devid,
  u8 *uuid)
 {
struct btrfs_device *dev;
-   struct list_head *cur;
 
-   list_for_each(cur, head) {
-   dev = list_entry(cur, struct btrfs_device, dev_list);
+   list_for_each_entry(dev, head, dev_list) {
if (dev->devid == devid &&
!memcmp(dev->uuid, uuid, BTRFS_UUID_SIZE)) {
return dev;
@@ -72,11 +70,9 @@ static struct btrfs_device *__find_device(struct list_head 
*head, u64 devid,
 
 static struct btrfs_fs_devices *find_fsid(u8 *fsid)
 {
-   struct list_head *cur;
struct btrfs_fs_devices *fs_devices;
 
-   list_for_each(cur, &fs_uuids) {
-   fs_devices = list_entry(cur, struct btrfs_fs_devices, list);
+   list_for_each_entry(fs_devices, &fs_uuids, list) {
if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0)
return fs_devices;
}
@@ -234,13 +230,10 @@ void btrfs_close_all_devices(void)
 int btrfs_open_devices(struct btrfs_fs_devices *fs_devices, int flags)
 {
int fd;
-   struct list_head *head = &fs_devices->devices;
-   struct list_head *cur;
struct btrfs_device *device;
int ret;
 
-   list_for_each(cur, head) {
-   device = list_entry(cur, struct btrfs_device, dev_list);
+   list_for_each_entry(device, &fs_devices->devices, dev_list) {
if (!device->name) {
printk("no name for device %llu, skip it now\n", 
device->devid);
continue;
@@ -1726,7 +1719,7 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
return -EIO;
}
if (btrfs_chunk_sector_size(leaf, chunk) != sectorsize) {
-   error("invalid chunk sectorsize %llu", 
+   error("invalid chunk sectorsize %llu",
  (unsigned long long)btrfs_chunk_sector_size(leaf, chunk));
return -EIO;
}
-- 
2.7.4

--
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:/

Re: [PATCH v2] btrfs-progs: Replace usage of list_for_each with list_for_each_entry

[Qu Wenruo]

On 2017年12月07日 17:10, Nikolay Borisov wrote:
> There are a couple of places where instead of the more succinct
> list_for_each_entry the code uses list_for_each. This results in
> slightly more code with no additional benefit as well as no
> coherent pattern. This patch makes the code uniform. No functional
> changes
> 
> Signed-off-by: Nikolay Borisov 

Reviewed-by: Qu Wenruo 

Thanks,
Qu

> ---
>  cmds-filesystem.c |  3 +--
>  disk-io.c |  4 +---
>  utils.c   |  7 +--
>  volumes.c | 15 ---
>  4 files changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 7728430f16a1..7c154589a15f 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -182,8 +182,7 @@ static int uuid_search(struct btrfs_fs_devices 
> *fs_devices, const char *search)
>   if (!strncmp(uuidbuf, search, search_len))
>   return 1;
>  
> - list_for_each(cur, &fs_devices->devices) {
> - device = list_entry(cur, struct btrfs_device, dev_list);
> + list_for_each_entry(device, &fs_devices->devices, dev_list) {
>   if ((device->label && strcmp(device->label, search) == 0) ||
>   strcmp(device->name, search) == 0)
>   return 1;
> diff --git a/disk-io.c b/disk-io.c
> index f5edc4796619..3d8785d5bb37 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -1556,7 +1556,6 @@ static int write_dev_supers(struct btrfs_fs_info 
> *fs_info,
>  
>  int write_all_supers(struct btrfs_fs_info *fs_info)
>  {
> - struct list_head *cur;
>   struct list_head *head = &fs_info->fs_devices->devices;
>   struct btrfs_device *dev;
>   struct btrfs_super_block *sb;
> @@ -1566,8 +1565,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info)
>  
>   sb = fs_info->super_copy;
>   dev_item = &sb->dev_item;
> - list_for_each(cur, head) {
> - dev = list_entry(cur, struct btrfs_device, dev_list);
> + list_for_each_entry(dev, head, dev_list) {
>   if (!dev->writeable)
>   continue;
>  
> diff --git a/utils.c b/utils.c
> index 6c0d9fc1bebf..65383282b512 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -804,14 +804,9 @@ static int blk_file_in_dev_list(struct btrfs_fs_devices* 
> fs_devices,
>   const char* file)
>  {
>   int ret;
> - struct list_head *head;
> - struct list_head *cur;
>   struct btrfs_device *device;
>  
> - head = &fs_devices->devices;
> - list_for_each(cur, head) {
> - device = list_entry(cur, struct btrfs_device, dev_list);
> -
> + list_for_each_entry(device, &fs_devices->devices, dev_list) {
>   if((ret = is_same_loop_file(device->name, file)))
>   return ret;
>   }
> diff --git a/volumes.c b/volumes.c
> index ce3a540578fd..2e1fb4a46465 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -58,10 +58,8 @@ static struct btrfs_device *__find_device(struct list_head 
> *head, u64 devid,
> u8 *uuid)
>  {
>   struct btrfs_device *dev;
> - struct list_head *cur;
>  
> - list_for_each(cur, head) {
> - dev = list_entry(cur, struct btrfs_device, dev_list);
> + list_for_each_entry(dev, head, dev_list) {
>   if (dev->devid == devid &&
>   !memcmp(dev->uuid, uuid, BTRFS_UUID_SIZE)) {
>   return dev;
> @@ -72,11 +70,9 @@ static struct btrfs_device *__find_device(struct list_head 
> *head, u64 devid,
>  
>  static struct btrfs_fs_devices *find_fsid(u8 *fsid)
>  {
> - struct list_head *cur;
>   struct btrfs_fs_devices *fs_devices;
>  
> - list_for_each(cur, &fs_uuids) {
> - fs_devices = list_entry(cur, struct btrfs_fs_devices, list);
> + list_for_each_entry(fs_devices, &fs_uuids, list) {
>   if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0)
>   return fs_devices;
>   }
> @@ -234,13 +230,10 @@ void btrfs_close_all_devices(void)
>  int btrfs_open_devices(struct btrfs_fs_devices *fs_devices, int flags)
>  {
>   int fd;
> - struct list_head *head = &fs_devices->devices;
> - struct list_head *cur;
>   struct btrfs_device *device;
>   int ret;
>  
> - list_for_each(cur, head) {
> - device = list_entry(cur, struct btrfs_device, dev_list);
> + list_for_each_entry(device, &fs_devices->devices, dev_list) {
>   if (!device->name) {
>   printk("no name for device %llu, skip it now\n", 
> device->devid);
>   continue;
> @@ -1726,7 +1719,7 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info 
> *fs_info,
>   return -EIO;
>   }
>   if (btrfs_chunk_sector_size(leaf, chunk) != sectorsize) {
> - error("invalid chunk sectorsize %llu", 
> + error("invalid chunk sectorsize %llu",
> (unsigned long long)btrfs_chunk_sector_size(leaf, chun

Re: [PATCH] btrfs/057: Fix test case to work on 64K page size

[Harish]

Sure will work on this. Thanks.

Harish

On 12/06/2017 01:16 PM, Qu Wenruo wrote:



On 2017年12月06日 15:35, Eryu Guan wrote:

On Wed, Dec 06, 2017 at 12:15:45PM +0530, Harish wrote:

On platforms with a page size greater than 4Kb, at the moment btrfs
doesn't support a node/leaf size smaller than the page size, but it
supports a larger one. So use the max supported node size (64Kb) so
that the test runs on any platform currently supported by Linux.

Signed-off-by: Harish 


Looks fine to me. Also cc btrfs list for review.

Thanks,
Eryu


---
 tests/btrfs/057 | 7 +--
 tests/btrfs/057.out | 4 ++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/tests/btrfs/057 b/tests/btrfs/057
index 8d35579..06b2be9 100755
--- a/tests/btrfs/057
+++ b/tests/btrfs/057
@@ -48,8 +48,11 @@ _require_scratch

 rm -f $seqres.full

-# use small leaf size to get higher btree height.
-run_check _scratch_mkfs "-b 1g --nodesize 4096"
+# Currently in btrfs the node/leaf size can not be smaller than the page
+# size (but it can be greater than the page size). So use the largest
+# supported node/leaf size (64Kb) so that the test can run on any platform
+# that Linux supports.
+run_check _scratch_mkfs "-b 1g --nodesize 65536"


Personally speaking, the test doesn't really need super high tree height.
So I think it's OK to use a different node size.

But I prefer default mkfs.btrfs, which will choose valid nodesize.
So for 4K system, it will use default 16K other than always using the
64K nodesize.


And further more, the test case is quite old, from the old days when we
did even have btrfs check support for qgroup.

With new btrfs check support, we don't really need to do anything to
check qgroup numbers in test cases, just let fstests post test routines
to handle it.

If qgroup is corrupted, fstests will report something inconsistent
filesystem at post test fsck.
Just like what we do in btrfs/126. (so no need to use any magic number
in 057.out)

So would you please also update the test case to get benefit from the
newer btrfs-progs?

Thanks,
Qu



 # inode cache is saved in the FS tree itself for every
 # individual FS tree,that affects the sizes reported by qgroup show
diff --git a/tests/btrfs/057.out b/tests/btrfs/057.out
index 60cb92d..f2d9e9c 100644
--- a/tests/btrfs/057.out
+++ b/tests/btrfs/057.out
@@ -1,3 +1,3 @@
 QA output created by 057
-4096 4096
-4096 4096
+65536 65536
+65536 65536
--
2.7.4

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

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





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

[RFC PATCH] btrfs: Handle btrfs_set_extent_delalloc failure in relocate_file_extent_cluster

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov 
---

Hello, 

THis is rfc since I'm not entirely sure the cleanup sequence currently is 
sufficient so ideas are welcome. I have the feeling we need to do something
with the page: 

* SetPageError
* ClearPageUptodate
* Something else? 


 fs/btrfs/relocation.c | 19 +++
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f0c3f00e97cb..b8614cd60ca6 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -3268,12 +3268,23 @@ static int relocate_file_extent_cluster(struct inode 
*inode,
nr++;
}
 
-   btrfs_set_extent_delalloc(inode, page_start, page_end, 0, NULL,
- 0);
+   ret = btrfs_set_extent_delalloc(inode, page_start, page_end, 0,
+   NULL, 0);
+   if (ret) {
+   unlock_page(page);
+   put_page(page);
+   btrfs_delalloc_release_metadata(BTRFS_I(inode),
+PAGE_SIZE);
+   btrfs_delalloc_release_extents(BTRFS_I(inode),
+  PAGE_SIZE);
+   unlock_extent(&BTRFS_I(inode)->io_tree, page_start,
+ page_end);
+   goto out;
+
+   }
set_page_dirty(page);
 
-   unlock_extent(&BTRFS_I(inode)->io_tree,
- page_start, page_end);
+   unlock_extent(&BTRFS_I(inode)->io_tree, page_start, page_end);
unlock_page(page);
put_page(page);
 
-- 
2.7.4

--
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: [RFC] Improve subvolume usability for a normal user

[Duncan]

Misono, Tomohiro posted on Thu, 07 Dec 2017 16:15:47 +0900 as excerpted:

> On 2017/12/07 11:56, Duncan wrote:
>> Austin S. Hemmelgarn posted on Wed, 06 Dec 2017 07:39:56 -0500 as
>> excerpted:
>> 
>>> Somewhat OT, but the only operation that's remotely 'instant' is
>>> creating an empty subvolume.  Snapshot creation has to walk the tree
>>> in the subvolume being snapshotted, which can take a long time (and as
>>> a result of it's implementation, also means BTRFS snapshots are _not_
>>> atomic). Subvolume deletion has to do a bunch of cleanup work in the
>>> background (though it may be fairly quick if it was a snapshot and the
>>> source subvolume hasn't changed much).
>> 
>> Indeed, while btrfs in general has taken a strategy of making
>> /creating/ snapshots and subvolumes fast, snapshot deletion in
>> particular can take some time[1].
>> 
>> And in that regard a question just occurred to me regarding this whole
>> very tough problem of a user being able to create but not delete
>> subvolumes and snapshots:
>> 
>> Given that at least snapshot deletion (not so sure about non-snapshot
>> subvolume deletion, tho I strongly suspect it would depend on the
>> number of cross-subvolume reflinks) is already a task that can take
>> some time, why /not/ just bite the bullet and make the behavior much
>> more like the directory deletion, given that subvolumes already behave
>> much like directories.  Yes, for non-root, that /does/ mean tracing the
>> entire subtree and checking permissions, and yes, that's going to take
>> time and lower performance somewhat, but subvolume and in particular
>> snapshot deletion is already an operation that takes time, so this
>> wouldn't be unduly changing the situation, and it would eliminate the
>> entire class of security issues that come with either asymmetrically
>> restricting deletion (but not creation) to root on the one hand,
> 
>> or possible data loss due to allowing a user to delete a subvolume they
>> couldn't delete were it an ordinary directory due to not owning stuff
>> further down the tree.
> 
> But, this is also the very reason I'm for "sub del" instead of unlink().
> Since snapshot creation won't check the permissions of the containing
> files/dirs, it can copy a directory which cannot be deleted by the user.
> Therefore if we won't allow "sub del" for the user, he couldn't remove
> the snapshot.

Maybe snapshot creation /should/ check all that, in ordered to allow 
permissions to allow deletion.

Tho that would unfortunately increase the creation time, and btrfs is 
currently optimized for fast creation time.

Hmm... What about creating a "temporary" snapshot if not root, then 
walking the tree to check perms and deleting it without ever showing it 
to userspace if the perms wouldn't let the user delete it.  That would 
retain fast creation logic, tho it wouldn't show up until the perms walk 
was completed.

-- 
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: [RFC] Improve subvolume usability for a normal user

[Austin S. Hemmelgarn]

On 2017-12-07 06:55, Duncan wrote:

Misono, Tomohiro posted on Thu, 07 Dec 2017 16:15:47 +0900 as excerpted:


On 2017/12/07 11:56, Duncan wrote:

Austin S. Hemmelgarn posted on Wed, 06 Dec 2017 07:39:56 -0500 as
excerpted:


Somewhat OT, but the only operation that's remotely 'instant' is
creating an empty subvolume.  Snapshot creation has to walk the tree
in the subvolume being snapshotted, which can take a long time (and as
a result of it's implementation, also means BTRFS snapshots are _not_
atomic). Subvolume deletion has to do a bunch of cleanup work in the
background (though it may be fairly quick if it was a snapshot and the
source subvolume hasn't changed much).


Indeed, while btrfs in general has taken a strategy of making
/creating/ snapshots and subvolumes fast, snapshot deletion in
particular can take some time[1].

And in that regard a question just occurred to me regarding this whole
very tough problem of a user being able to create but not delete
subvolumes and snapshots:

Given that at least snapshot deletion (not so sure about non-snapshot
subvolume deletion, tho I strongly suspect it would depend on the
number of cross-subvolume reflinks) is already a task that can take
some time, why /not/ just bite the bullet and make the behavior much
more like the directory deletion, given that subvolumes already behave
much like directories.  Yes, for non-root, that /does/ mean tracing the
entire subtree and checking permissions, and yes, that's going to take
time and lower performance somewhat, but subvolume and in particular
snapshot deletion is already an operation that takes time, so this
wouldn't be unduly changing the situation, and it would eliminate the
entire class of security issues that come with either asymmetrically
restricting deletion (but not creation) to root on the one hand,



or possible data loss due to allowing a user to delete a subvolume they
couldn't delete were it an ordinary directory due to not owning stuff
further down the tree.


But, this is also the very reason I'm for "sub del" instead of unlink().
Since snapshot creation won't check the permissions of the containing
files/dirs, it can copy a directory which cannot be deleted by the user.
Therefore if we won't allow "sub del" for the user, he couldn't remove
the snapshot.


Maybe snapshot creation /should/ check all that, in ordered to allow
permissions to allow deletion.

Tho that would unfortunately increase the creation time, and btrfs is
currently optimized for fast creation time.

Hmm... What about creating a "temporary" snapshot if not root, then
walking the tree to check perms and deleting it without ever showing it
to userspace if the perms wouldn't let the user delete it.  That would
retain fast creation logic, tho it wouldn't show up until the perms walk
was completed.

I would argue that it makes more sense to keep snapshot creation as is, 
keep the subvolume deletion command as is (with some proper permissions 
checks of course), and just make unlink() work for subvolumes like it 
does for directories.

--
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: [RFC] Improve subvolume usability for a normal user

[Hugo Mills]

On Thu, Dec 07, 2017 at 07:21:46AM -0500, Austin S. Hemmelgarn wrote:
> On 2017-12-07 06:55, Duncan wrote:
> >Misono, Tomohiro posted on Thu, 07 Dec 2017 16:15:47 +0900 as excerpted:
> >
> >>On 2017/12/07 11:56, Duncan wrote:
> >>>Austin S. Hemmelgarn posted on Wed, 06 Dec 2017 07:39:56 -0500 as
> >>>excerpted:
> >>>
> Somewhat OT, but the only operation that's remotely 'instant' is
> creating an empty subvolume.  Snapshot creation has to walk the tree
> in the subvolume being snapshotted, which can take a long time (and as
> a result of it's implementation, also means BTRFS snapshots are _not_
> atomic). Subvolume deletion has to do a bunch of cleanup work in the
> background (though it may be fairly quick if it was a snapshot and the
> source subvolume hasn't changed much).
> >>>
> >>>Indeed, while btrfs in general has taken a strategy of making
> >>>/creating/ snapshots and subvolumes fast, snapshot deletion in
> >>>particular can take some time[1].
> >>>
> >>>And in that regard a question just occurred to me regarding this whole
> >>>very tough problem of a user being able to create but not delete
> >>>subvolumes and snapshots:
> >>>
> >>>Given that at least snapshot deletion (not so sure about non-snapshot
> >>>subvolume deletion, tho I strongly suspect it would depend on the
> >>>number of cross-subvolume reflinks) is already a task that can take
> >>>some time, why /not/ just bite the bullet and make the behavior much
> >>>more like the directory deletion, given that subvolumes already behave
> >>>much like directories.  Yes, for non-root, that /does/ mean tracing the
> >>>entire subtree and checking permissions, and yes, that's going to take
> >>>time and lower performance somewhat, but subvolume and in particular
> >>>snapshot deletion is already an operation that takes time, so this
> >>>wouldn't be unduly changing the situation, and it would eliminate the
> >>>entire class of security issues that come with either asymmetrically
> >>>restricting deletion (but not creation) to root on the one hand,
> >>
> >>>or possible data loss due to allowing a user to delete a subvolume they
> >>>couldn't delete were it an ordinary directory due to not owning stuff
> >>>further down the tree.
> >>
> >>But, this is also the very reason I'm for "sub del" instead of unlink().
> >>Since snapshot creation won't check the permissions of the containing
> >>files/dirs, it can copy a directory which cannot be deleted by the user.
> >>Therefore if we won't allow "sub del" for the user, he couldn't remove
> >>the snapshot.
> >
> >Maybe snapshot creation /should/ check all that, in ordered to allow
> >permissions to allow deletion.
> >
> >Tho that would unfortunately increase the creation time, and btrfs is
> >currently optimized for fast creation time.
> >
> >Hmm... What about creating a "temporary" snapshot if not root, then
> >walking the tree to check perms and deleting it without ever showing it
> >to userspace if the perms wouldn't let the user delete it.  That would
> >retain fast creation logic, tho it wouldn't show up until the perms walk
> >was completed.
> >
> I would argue that it makes more sense to keep snapshot creation as
> is, keep the subvolume deletion command as is (with some proper
> permissions checks of course), and just make unlink() work for
> subvolumes like it does for directories.

   Definitely this.

   Principle of least surprise.

   Hugo.

-- 
Hugo Mills | ... one ping(1) to rule them all, and in the
hugo@... carfax.org.uk | darkness bind(2) them.
http://carfax.org.uk/  |
PGP: E2AB1DE4  |Illiad


signature.asc
Description: Digital signature

[PATCH v5 1/3] btrfs: add function to device list delete

[Anand Jain]

We need device delete from the dev_list so create a new function.
New instead of refactor of btrfs_free_stale_device() because,
btrfs_free_stale_device() doesn't hold device_list_mutex which
is actually needed here.

Signed-off-by: Anand Jain 
---
v1: title of this patch
  btrfs: refactor btrfs_free_stale_device() to get device list delete
v2:
 delete_device_from_list() is not pealed from btrfs_free_stale_device()
 as this needs device_list_mutex. And its static now.
v3: Send to correct ML
v4: no change
v5: use helper free_device()

 fs/btrfs/volumes.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d0dd0eb3d2e5..f53d62e92fa9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -607,6 +607,26 @@ static void pending_bios_fn(struct btrfs_work *work)
run_scheduled_bios(device);
 }
 
+static void delete_device_from_list(struct btrfs_device *device)
+{
+   struct btrfs_fs_devices *fs_devices;
+
+   fs_devices = device->fs_devices;
+
+   lockdep_assert_held(&uuid_mutex);
+
+   mutex_lock(&fs_devices->device_list_mutex);
+   fs_devices->num_devices--;
+   list_del(&device->dev_list);
+   mutex_unlock(&fs_devices->device_list_mutex);
+
+   free_device(device);
+   if (fs_devices->num_devices == 0) {
+   btrfs_sysfs_remove_fsid(fs_devices);
+   list_del(&fs_devices->list);
+   free_fs_devices(fs_devices);
+   }
+}
 
 static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 {
-- 
2.7.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 v5 0/3] Add cli and ioctl to deregister devices

[Anand Jain]

Add ability to deregister a or all devices. I have named this sub cmd
as deregister, but I am open to your suggestions.

Further I am using /dev/btrfs-control and struct btrfs_ioctl_vol_args
for now, just similar to other ioctls in super.c including btrfs dev
scan, if rather it makes sense to use btrfs_ioctl_vol_args_v2 I shall.
Oepn to suggestions.

Kernel:
Patch 1/3 is preparatory patch, provides helper to delete a device.
Patch 2/3 adds ioctl to delete device from the device list.
Patch 3/3 provides default feature to delete all fs_devices in the list.

btrfs-progs:
Patch 1/2 introduce 'btrfs dev deregister '
Patch 2/2 make the device optional and when not provided, deregisters all
unmounted devices.

v2:
  Accepted review from Nikolay, details are in the specific patch.
  Patch 1/2 is renamed from
[PATCH 1/2] btrfs: refactor btrfs_free_stale_device() to get device list 
delete
  to
[PATCH 1/2] btrfs: add function to device list delete
v3:
  No change. Send to correct ML.
v4:
  No change. But as the ML thread may be confusing, so resend.
v5:
  Mainly adds feature to deregister all unmounted devices.
  Accepts review from David, for details ref change log.

Anand Jain (3):
  btrfs: add function to device list delete
  btrfs: introduce feature to deregister a btrfs device
  btrfs: add feature to deregister all unmounted devices

 fs/btrfs/super.c   |   9 
 fs/btrfs/volumes.c | 119 +
 fs/btrfs/volumes.h |   3 ++
 include/uapi/linux/btrfs.h |   9 +++-
 4 files changed, 138 insertions(+), 2 deletions(-)

Anand Jain (2):
  btrfs-progs: add 'btrfs device unregister' cli
  btrfs-progs: add feature to deregister all devices

 cmds-device.c | 82 +++
 ioctl.h   |  8 +-
 2 files changed, 89 insertions(+), 1 deletion(-)

-- 
2.7.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 v5 2/3] btrfs: introduce feature to deregister a btrfs device

[Anand Jain]

Support for a new command is being added here:
 btrfs dev deregister 
Which shall undo the effects of the command
 btrfs dev scan 

This cli/ioctl is needed as there is no way to continue to mount in
degraded mode if the device is already scanned, which is required to
recover from the split brain raid conditions.

This patch proposes to use ioctl #5 as it was empty.
IOW(BTRFS_IOCTL_MAGIC, 5, ..)

Signed-off-by: Anand Jain 
---
v2: Use -EBUSY instead of -ENOENT
Since now delete_device_from_list() holds device_list_mutex
so dont hold device_list_mutex in its parent. Reword and indent
pr_err/info.
v3: Send to correct ML
v4: no change.
v5: Make sure uuid_mutex in device_list_remove(), set ret value to near
to the event.

 fs/btrfs/super.c   |  4 +++
 fs/btrfs/volumes.c | 76 ++
 fs/btrfs/volumes.h |  2 ++
 include/uapi/linux/btrfs.h |  3 +-
 4 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f443517fa2f8..ea6fe4742834 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2212,6 +2212,10 @@ static long btrfs_control_ioctl(struct file *file, 
unsigned int cmd,
ret = btrfs_scan_one_device(vol->name, FMODE_READ,
&btrfs_fs_type, &fs_devices);
break;
+   case BTRFS_IOC_PURGE_DEV:
+   ret = btrfs_purge_one_device(vol->name, FMODE_READ,
+   &btrfs_fs_type, &fs_devices);
+   break;
case BTRFS_IOC_DEVICES_READY:
ret = btrfs_scan_one_device(vol->name, FMODE_READ,
&btrfs_fs_type, &fs_devices);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f53d62e92fa9..5adab70c7658 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1260,6 +1260,82 @@ static int btrfs_read_disk_super(struct block_device 
*bdev, u64 bytenr,
return 0;
 }
 
+static int device_list_remove(struct btrfs_super_block *disk_super, u64 devid)
+{
+   struct btrfs_fs_devices *fs_devices;
+   struct btrfs_device *device;
+   int ret;
+
+   mutex_lock(&uuid_mutex);
+   fs_devices = find_fsid(disk_super->fsid);
+   if (!fs_devices) {
+   ret = -ENOENT;
+   goto out;
+   }
+
+   if (fs_devices->opened) {
+   ret = -EBUSY;
+   goto out;
+   }
+
+   device = find_device(fs_devices, devid, disk_super->dev_item.uuid);
+   if (!device) {
+   ret = -ENOENT;
+   goto out;
+   }
+
+   delete_device_from_list(device);
+   ret = 0;
+out:
+   mutex_unlock(&uuid_mutex);
+
+   return ret;
+}
+
+int btrfs_purge_one_device(const char *path, fmode_t flags, void *holder,
+ struct btrfs_fs_devices **fs_devices_ret)
+{
+   struct btrfs_super_block *disk_super;
+   struct block_device *bdev;
+   struct page *page;
+   u64 bytenr;
+   u64 devid;
+   int ret;
+
+   bytenr = btrfs_sb_offset(0);
+   flags |= FMODE_EXCL;
+
+   bdev = blkdev_get_by_path(path, flags, holder);
+   if (IS_ERR(bdev))
+   return PTR_ERR(bdev);
+
+   if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super)) {
+   ret = -EINVAL;
+   goto error_bdev_put;
+   }
+
+   devid = btrfs_stack_device_id(&disk_super->dev_item);
+
+   ret = device_list_remove(disk_super, devid);
+   /* User error */
+   if (ret == -ENOENT)
+   goto error;
+
+   if (ret)
+   pr_err("BTRFS: %pU device %s devid %llu failed to deregister: 
%d\n",
+   disk_super->fsid, path, devid, ret);
+   else
+   pr_info("BTRFS: %pU device %s devid %llu deregistered\n",
+   disk_super->fsid, path, devid);
+
+error:
+   btrfs_release_disk_super(page);
+
+error_bdev_put:
+   blkdev_put(bdev, flags);
+   return ret;
+}
+
 /*
  * Look for a btrfs signature on a device. This may be called out of the mount 
path
  * and we are not allowed to call set_blocksize during the scan. The superblock
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index efb5117301b7..8389d2815750 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -422,6 +422,8 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
   fmode_t flags, void *holder);
 int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
  struct btrfs_fs_devices **fs_devices_ret);
+int btrfs_purge_one_device(const char *path, fmode_t flags, void *holder,
+ struct btrfs_fs_devices **fs_devices_ret);
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step);
 void btrfs_assign_next_active_device(struct btrfs_fs_inf

[PATCH v5 3/3] btrfs: add feature to deregister all unmounted devices

[Anand Jain]

This adds a feature to remove all registered/scanned devices,
which are not mounted or it got staled by some means.

Signed-off-by: Anand Jain 
Suggested-by: David Sterba 
---
v2-4: Does not exist.

 fs/btrfs/super.c   |  7 ++-
 fs/btrfs/volumes.c | 23 +++
 fs/btrfs/volumes.h |  1 +
 include/uapi/linux/btrfs.h |  6 +-
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index ea6fe4742834..409804843b74 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2213,7 +2213,12 @@ static long btrfs_control_ioctl(struct file *file, 
unsigned int cmd,
&btrfs_fs_type, &fs_devices);
break;
case BTRFS_IOC_PURGE_DEV:
-   ret = btrfs_purge_one_device(vol->name, FMODE_READ,
+   if (vol->ioctl_flag & ~BTRFS_IOCTL_PURGE_ALL_DEVS)
+   return -EOPNOTSUPP;
+   if (vol->ioctl_flag & BTRFS_IOCTL_PURGE_ALL_DEVS)
+   ret = btrfs_purge_devices();
+   else
+   ret = btrfs_purge_one_device(vol->name, FMODE_READ,
&btrfs_fs_type, &fs_devices);
break;
case BTRFS_IOC_DEVICES_READY:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5adab70c7658..9fa2539a8493 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1336,6 +1336,29 @@ int btrfs_purge_one_device(const char *path, fmode_t 
flags, void *holder,
return ret;
 }
 
+int btrfs_purge_devices(void)
+{
+   struct btrfs_fs_devices *fs_devices, *tmp;
+
+   mutex_lock(&uuid_mutex);
+   list_for_each_entry_safe(fs_devices, tmp, &fs_uuids, list) {
+   /*
+* For now ignore stale device within a mounted FS.
+*/
+   if (fs_devices->opened)
+   continue;
+
+   pr_info("BTRFS: %pU num_devices %llu deregistered",
+   fs_devices->fsid, fs_devices->num_devices);
+   btrfs_sysfs_remove_fsid(fs_devices);
+   list_del(&fs_devices->list);
+   free_fs_devices(fs_devices);
+   }
+   mutex_unlock(&uuid_mutex);
+
+   return 0;
+}
+
 /*
  * Look for a btrfs signature on a device. This may be called out of the mount 
path
  * and we are not allowed to call set_blocksize during the scan. The superblock
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 8389d2815750..dec27ee4f67d 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -424,6 +424,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, 
void *holder,
  struct btrfs_fs_devices **fs_devices_ret);
 int btrfs_purge_one_device(const char *path, fmode_t flags, void *holder,
  struct btrfs_fs_devices **fs_devices_ret);
+int btrfs_purge_devices(void);
 int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step);
 void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index de0f1144d945..eaf6ef04b300 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -28,8 +28,12 @@
 
 /* this should be 4k */
 #define BTRFS_PATH_NAME_MAX 4087
+#define BTRFS_IOCTL_PURGE_ALL_DEVS (1ULL << 0)
 struct btrfs_ioctl_vol_args {
-   __s64 fd;
+   union {
+   __s64 fd;
+   __u64 ioctl_flag;
+   };
char name[BTRFS_PATH_NAME_MAX + 1];
 };
 
-- 
2.7.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 v5 1/2] btrfs-progs: add cli to deregister a device

[Anand Jain]

This patch adds
  btrfs device deregister 
so that an already registered device (through scan) can be deregistered.

This change is compatible with older kernel without the ioctl
BTRFS_IOC_PURGE_DEV it shall report 'Inappropriate ioctl for device'.

Signed-off-by: Anand Jain 
---
v1-4: No change.
v5: make provision to add no option to delete all fsids

 cmds-device.c | 55 +++
 ioctl.h   |  2 ++
 2 files changed, 57 insertions(+)

diff --git a/cmds-device.c b/cmds-device.c
index f4cdb39f64ac..8244ff9802be 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -329,6 +329,60 @@ out:
return !!ret;
 }
 
+static const char * const cmd_device_deregister_usage[] = {
+   "btrfs device deregister []",
+   "Deregister device in btrfs kernel module.",
+   NULL
+};
+
+static int btrfs_deregister_one_device(char *path)
+{
+   struct btrfs_ioctl_vol_args args;
+   int fd;
+   int ret;
+
+   fd = open("/dev/btrfs-control", O_RDWR);
+   if (fd < 0)
+   return -errno;
+
+   memset(&args, 0, sizeof(args));
+   strncpy_null(args.name, path);
+   ret = ioctl(fd, BTRFS_IOC_DEREGISTER_DEV, &args);
+   if (ret)
+   ret = -errno;
+   close(fd);
+   return ret;
+}
+
+static int cmd_device_deregister(int argc, char **argv)
+{
+   char *path;
+   int ret = 0;
+
+   if (check_argc_exact(argc - optind, 1))
+   usage(cmd_device_deregister_usage);
+
+   if (is_block_device(argv[1]) != 1) {
+   error("Not a block device: %s", argv[1]);
+   return -ENOENT;
+   }
+
+   path = canonicalize_path(argv[1]);
+   if (!path) {
+   error("Could not canonicalize path '%s': %s",
+   argv[1], strerror(errno));
+   return -ENOENT;
+   }
+
+   ret  = btrfs_deregister_one_device(path);
+   if (ret)
+   error("Can't deregister '%s': %s", path, strerror(-ret));
+
+   free(path);
+
+   return ret;
+}
+
 static const char * const cmd_device_ready_usage[] = {
"btrfs device ready ",
"Check device to see if it has all of its devices in cache for 
mounting",
@@ -604,6 +658,7 @@ const struct cmd_group device_cmd_group = {
CMD_ALIAS },
{ "remove", cmd_device_remove, cmd_device_remove_usage, NULL, 0 
},
{ "scan", cmd_device_scan, cmd_device_scan_usage, NULL, 0 },
+   { "deregister", cmd_device_deregister, 
cmd_device_deregister_usage, NULL, 0 },
{ "ready", cmd_device_ready, cmd_device_ready_usage, NULL, 0 },
{ "stats", cmd_device_stats, cmd_device_stats_usage, NULL, 0 },
{ "usage", cmd_device_usage,
diff --git a/ioctl.h b/ioctl.h
index 709e996f401c..3cdb88eb5bb2 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -721,6 +721,8 @@ static inline char *btrfs_err_str(enum btrfs_err_code 
err_code)
   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_SCAN_DEV _IOW(BTRFS_IOCTL_MAGIC, 4, \
   struct btrfs_ioctl_vol_args)
+#define BTRFS_IOC_DEREGISTER_DEV _IOW(BTRFS_IOCTL_MAGIC, 5, \
+  struct btrfs_ioctl_vol_args)
 /* trans start and trans end are dangerous, and only for
  * use by applications that know how to avoid the
  * resulting deadlocks
-- 
2.7.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 v5 2/2] btrfs-progs: add feature to deregister all devices

[Anand Jain]

This patch adds default no option to deregister all unmounted
devices in the kernel at once. For example:
  btrfs device deregister

Signed-off-by: Anand Jain 
---
v1-4: NA.
v5: New patch in v5.

 cmds-device.c | 31 +--
 ioctl.h   |  6 +-
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 8244ff9802be..6b9c3616b641 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -331,10 +331,30 @@ out:
 
 static const char * const cmd_device_deregister_usage[] = {
"btrfs device deregister []",
-   "Deregister device in btrfs kernel module.",
+   "Deregister one or all scanned device(s) in the kernel.",
NULL
 };
 
+static int btrfs_deregister_devices(void)
+{
+   struct btrfs_ioctl_vol_args args;
+   int fd;
+   int ret;
+
+   fd = open("/dev/btrfs-control", O_RDWR);
+   if (fd < 0)
+   return -errno;
+
+   memset(&args, 0, sizeof(args));
+   args.ioctl_flag = BTRFS_IOCTL_PURGE_ALL_DEVS;
+   ret = ioctl(fd, BTRFS_IOC_DEREGISTER_DEV, &args);
+   if (ret)
+   ret = -errno;
+   close(fd);
+   return ret;
+
+}
+
 static int btrfs_deregister_one_device(char *path)
 {
struct btrfs_ioctl_vol_args args;
@@ -359,9 +379,16 @@ static int cmd_device_deregister(int argc, char **argv)
char *path;
int ret = 0;
 
-   if (check_argc_exact(argc - optind, 1))
+   if (check_argc_max(argc - optind, 1))
usage(cmd_device_deregister_usage);
 
+   if (argc == 1) {
+   ret = btrfs_deregister_devices();
+   if (ret)
+   error("Can't deregister: %s", strerror(-ret));
+   return ret;
+   }
+
if (is_block_device(argv[1]) != 1) {
error("Not a block device: %s", argv[1]);
return -ENOENT;
diff --git a/ioctl.h b/ioctl.h
index 3cdb88eb5bb2..a36055b453e2 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -41,8 +41,12 @@ extern "C" {
 
 /* this should be 4k */
 #define BTRFS_PATH_NAME_MAX 4087
+#define BTRFS_IOCTL_PURGE_ALL_DEVS (1ULL << 0)
 struct btrfs_ioctl_vol_args {
-   __s64 fd;
+   union {
+   __s64 fd;
+   __u64 ioctl_flag;
+   };
char name[BTRFS_PATH_NAME_MAX + 1];
 };
 BUILD_ASSERT(sizeof(struct btrfs_ioctl_vol_args) == 4096);
-- 
2.7.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 v5 0/3] Add cli and ioctl to deregister devices

[Austin S. Hemmelgarn]

On 2017-12-07 09:36, Anand Jain wrote:

Add ability to deregister a or all devices. I have named this sub cmd
as deregister, but I am open to your suggestions.
Being a bit picky here, but from the perspective of a native speaker of 
American English, I would say that 'deregister' sounds rather synthetic 
and somewhat harsh and alien.


Given that, as odd as it sounds, I think 'ignore' might be a more user 
friendly name for the sub-command.  It accurately describes what the 
command is doing (telling the kernel to ignore the device), and it's a 
lot less alien sounding than 'deregister'.


If you're set on having it be based on the word 'register', I would 
suggest changing it to 'unregister', as I think that sounds more natural 
than 'deregister'.


Additionally, if you do continue with 'deregister' or go with 
'unregister' as the name though, I would suggest adding 'register' as a 
synonym for the 'scan' sub-command to keep things reasonably symmetrical.


Further I am using /dev/btrfs-control and struct btrfs_ioctl_vol_args
for now, just similar to other ioctls in super.c including btrfs dev
scan, if rather it makes sense to use btrfs_ioctl_vol_args_v2 I shall.
Oepn to suggestions.

Kernel:
Patch 1/3 is preparatory patch, provides helper to delete a device.
Patch 2/3 adds ioctl to delete device from the device list.
Patch 3/3 provides default feature to delete all fs_devices in the list.

btrfs-progs:
Patch 1/2 introduce 'btrfs dev deregister '
Patch 2/2 make the device optional and when not provided, deregisters all
unmounted devices.

v2:
   Accepted review from Nikolay, details are in the specific patch.
   Patch 1/2 is renamed from
 [PATCH 1/2] btrfs: refactor btrfs_free_stale_device() to get device list 
delete
   to
 [PATCH 1/2] btrfs: add function to device list delete
v3:
   No change. Send to correct ML.
v4:
   No change. But as the ML thread may be confusing, so resend.
v5:
   Mainly adds feature to deregister all unmounted devices.
   Accepts review from David, for details ref change log.

Anand Jain (3):
   btrfs: add function to device list delete
   btrfs: introduce feature to deregister a btrfs device
   btrfs: add feature to deregister all unmounted devices

  fs/btrfs/super.c   |   9 
  fs/btrfs/volumes.c | 119 +
  fs/btrfs/volumes.h |   3 ++
  include/uapi/linux/btrfs.h |   9 +++-
  4 files changed, 138 insertions(+), 2 deletions(-)

Anand Jain (2):
   btrfs-progs: add 'btrfs device unregister' cli
   btrfs-progs: add feature to deregister all devices

  cmds-device.c | 82 +++
  ioctl.h   |  8 +-
  2 files changed, 89 insertions(+), 1 deletion(-)



--
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] btrfs: tree-checker: use %zu format string for size_t

[David Sterba]

On Thu, Dec 07, 2017 at 08:32:04AM +0800, Qu Wenruo wrote:
> 
> 
> On 2017年12月06日 22:18, Arnd Bergmann wrote:
> > The return value of sizeof() is of type size_t, so we must print it
> > using the %z format modifier rather than %l to avoid this warning
> > on some architectures:
> > 
> > fs/btrfs/tree-checker.c: In function 'check_dir_item':
> > fs/btrfs/tree-checker.c:273:50: error: format '%lu' expects argument of 
> > type 'long unsigned int', but argument 5 has type 'u32' {aka 'unsigned 
> > int'} [-Werror=format=]
> 
> Any idea about which architecture will cause such warning?
> On x86_64 I always fail to get such warning.

The intel 0-day build bot compiles on various architectures and 32/64
setups, I'm not sure if this warning has been reported, no such mail in
my mboxes.
--
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 v4 72/73] xfs: Convert mru cache to XArray

[Theodore Ts'o]

On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote:
> > Unfortunately for you, I don't find arguments along the lines of
> > "lockdep will save us" at all convincing.  lockdep already throws
> > too many false positives to be useful as a tool that reliably and
> > accurately points out rare, exciting, complex, intricate locking
> > problems.
> 
> But it does reliably and accurately point out "dude, you forgot to take
> the lock".  It's caught a number of real problems in my own testing that
> you never got to see.

The problem is that if it has too many false positives --- and it's
gotten *way* worse with the completion callback "feature", people will
just stop using Lockdep as being too annyoing and a waste of developer
time when trying to figure what is a legitimate locking bug versus
lockdep getting confused.

I can't even disable the new Lockdep feature which is throwing
lots of new false positives --- it's just all or nothing.

Dave has just said he's already stopped using Lockdep, as a result.

  - Ted
--
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/4] btrfs: unify extent_page_data type passed as void

[David Sterba]

Functions called from extent_write_cache_pages used void* as generic
callback data, but all of them convert it to extent_page_data, or use it
directly.

Signed-off-by: David Sterba 
---
 fs/btrfs/extent_io.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 47435256a8c3..6cfb25c30a2d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -138,7 +138,7 @@ static void add_extent_changeset(struct extent_state 
*state, unsigned bits,
BUG_ON(ret < 0);
 }
 
-static void flush_write_bio(void *data);
+static void flush_write_bio(struct extent_page_data *epd);
 
 static inline struct btrfs_fs_info *
 tree_fs_info(struct extent_io_tree *tree)
@@ -3456,10 +3456,9 @@ static noinline_for_stack int 
__extent_writepage_io(struct inode *inode,
  * and the end_io handler clears the writeback ranges
  */
 static int __extent_writepage(struct page *page, struct writeback_control *wbc,
- void *data)
+ struct extent_page_data *epd)
 {
struct inode *inode = page->mapping->host;
-   struct extent_page_data *epd = data;
u64 start = page_offset(page);
u64 page_end = start + PAGE_SIZE - 1;
int ret;
@@ -3905,7 +3904,7 @@ int btree_write_cache_pages(struct address_space *mapping,
  */
 static int extent_write_cache_pages(struct address_space *mapping,
 struct writeback_control *wbc,
-void *data)
+struct extent_page_data *epd)
 {
struct inode *inode = mapping->host;
int ret = 0;
@@ -3969,7 +3968,7 @@ static int extent_write_cache_pages(struct address_space 
*mapping,
 * mapping
 */
if (!trylock_page(page)) {
-   flush_write_bio(data);
+   flush_write_bio(epd);
lock_page(page);
}
 
@@ -3980,7 +3979,7 @@ static int extent_write_cache_pages(struct address_space 
*mapping,
 
if (wbc->sync_mode != WB_SYNC_NONE) {
if (PageWriteback(page))
-   flush_write_bio(data);
+   flush_write_bio(epd);
wait_on_page_writeback(page);
}
 
@@ -3990,7 +3989,7 @@ static int extent_write_cache_pages(struct address_space 
*mapping,
continue;
}
 
-   ret = __extent_writepage(page, wbc, data);
+   ret = __extent_writepage(page, wbc, epd);
 
if (unlikely(ret == AOP_WRITEPAGE_ACTIVATE)) {
unlock_page(page);
@@ -4038,10 +4037,8 @@ static int extent_write_cache_pages(struct address_space 
*mapping,
return ret;
 }
 
-static void flush_write_bio(void *data)
+static void flush_write_bio(struct extent_page_data *epd)
 {
-   struct extent_page_data *epd = data;
-
if (epd->bio) {
int ret;
 
-- 
2.15.1

--
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/4] btrfs: sink flush_fn to extent_write_cache_pages

[David Sterba]

All callers pass the same value flush_write_bio.

Signed-off-by: David Sterba 
---
 fs/btrfs/extent_io.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fd1a2ff1e96d..35f4869a90d5 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3906,8 +3906,7 @@ int btree_write_cache_pages(struct address_space *mapping,
  */
 static int extent_write_cache_pages(struct address_space *mapping,
 struct writeback_control *wbc,
-writepage_t writepage, void *data,
-void (*flush_fn)(void *))
+writepage_t writepage, void *data)
 {
struct inode *inode = mapping->host;
int ret = 0;
@@ -3971,7 +3970,7 @@ static int extent_write_cache_pages(struct address_space 
*mapping,
 * mapping
 */
if (!trylock_page(page)) {
-   flush_fn(data);
+   flush_write_bio(data);
lock_page(page);
}
 
@@ -3982,7 +3981,7 @@ static int extent_write_cache_pages(struct address_space 
*mapping,
 
if (wbc->sync_mode != WB_SYNC_NONE) {
if (PageWriteback(page))
-   flush_fn(data);
+   flush_write_bio(data);
wait_on_page_writeback(page);
}
 
@@ -4123,8 +4122,7 @@ int extent_writepages(struct extent_io_tree *tree,
.sync_io = wbc->sync_mode == WB_SYNC_ALL,
};
 
-   ret = extent_write_cache_pages(mapping, wbc, __extent_writepage, &epd,
-  flush_write_bio);
+   ret = extent_write_cache_pages(mapping, wbc, __extent_writepage, &epd);
flush_write_bio(&epd);
return ret;
 }
-- 
2.15.1

--
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/4] btrfs: sink writepage parameter to extent_write_cache_pages

[David Sterba]

The function extent_write_cache_pages is modelled after
write_cache_pages which is a generic interface and the writepage
parameter makes sense there. In btrfs we know exactly which callback
we're going to use, so we can pass it directly.

Signed-off-by: David Sterba 
---
 fs/btrfs/extent_io.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 35f4869a90d5..47435256a8c3 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3893,8 +3893,7 @@ int btree_write_cache_pages(struct address_space *mapping,
  * write_cache_pages - walk the list of dirty pages of the given address space 
and write all of them.
  * @mapping: address space structure to write
  * @wbc: subtract the number of written pages from *@wbc->nr_to_write
- * @writepage: function called for each page
- * @data: data passed to writepage function
+ * @data: data passed to __extent_writepage function
  *
  * If a page is already under I/O, write_cache_pages() skips it, even
  * if it's dirty.  This is desirable behaviour for memory-cleaning writeback,
@@ -3906,7 +3905,7 @@ int btree_write_cache_pages(struct address_space *mapping,
  */
 static int extent_write_cache_pages(struct address_space *mapping,
 struct writeback_control *wbc,
-writepage_t writepage, void *data)
+void *data)
 {
struct inode *inode = mapping->host;
int ret = 0;
@@ -3991,7 +3990,7 @@ static int extent_write_cache_pages(struct address_space 
*mapping,
continue;
}
 
-   ret = (*writepage)(page, wbc, data);
+   ret = __extent_writepage(page, wbc, data);
 
if (unlikely(ret == AOP_WRITEPAGE_ACTIVATE)) {
unlock_page(page);
@@ -4122,7 +4121,7 @@ int extent_writepages(struct extent_io_tree *tree,
.sync_io = wbc->sync_mode == WB_SYNC_ALL,
};
 
-   ret = extent_write_cache_pages(mapping, wbc, __extent_writepage, &epd);
+   ret = extent_write_cache_pages(mapping, wbc, &epd);
flush_write_bio(&epd);
return ret;
 }
-- 
2.15.1

--
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/4] btrfs: merge two flush_write_bio helpers

[David Sterba]

flush_epd_write_bio is same as flush_write_bio, no point having two such
functions. Merge them to flush_write_bio. The 'noinline' attribute is
removed as it does not have any meaning.

Signed-off-by: David Sterba 
---
 fs/btrfs/extent_io.c | 19 ---
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 16ae832bdb5d..fd1a2ff1e96d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -138,7 +138,8 @@ static void add_extent_changeset(struct extent_state 
*state, unsigned bits,
BUG_ON(ret < 0);
 }
 
-static noinline void flush_write_bio(void *data);
+static void flush_write_bio(void *data);
+
 static inline struct btrfs_fs_info *
 tree_fs_info(struct extent_io_tree *tree)
 {
@@ -4039,8 +4040,10 @@ static int extent_write_cache_pages(struct address_space 
*mapping,
return ret;
 }
 
-static void flush_epd_write_bio(struct extent_page_data *epd)
+static void flush_write_bio(void *data)
 {
+   struct extent_page_data *epd = data;
+
if (epd->bio) {
int ret;
 
@@ -4050,12 +4053,6 @@ static void flush_epd_write_bio(struct extent_page_data 
*epd)
}
 }
 
-static noinline void flush_write_bio(void *data)
-{
-   struct extent_page_data *epd = data;
-   flush_epd_write_bio(epd);
-}
-
 int extent_write_full_page(struct extent_io_tree *tree, struct page *page,
  struct writeback_control *wbc)
 {
@@ -4069,7 +4066,7 @@ int extent_write_full_page(struct extent_io_tree *tree, 
struct page *page,
 
ret = __extent_writepage(page, wbc, &epd);
 
-   flush_epd_write_bio(&epd);
+   flush_write_bio(&epd);
return ret;
 }
 
@@ -4110,7 +4107,7 @@ int extent_write_locked_range(struct extent_io_tree 
*tree, struct inode *inode,
start += PAGE_SIZE;
}
 
-   flush_epd_write_bio(&epd);
+   flush_write_bio(&epd);
return ret;
 }
 
@@ -4128,7 +4125,7 @@ int extent_writepages(struct extent_io_tree *tree,
 
ret = extent_write_cache_pages(mapping, wbc, __extent_writepage, &epd,
   flush_write_bio);
-   flush_epd_write_bio(&epd);
+   flush_write_bio(&epd);
return ret;
 }
 
-- 
2.15.1

--
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/4] Cleanups for extent_page_data

[David Sterba]

David Sterba (4):
  btrfs: merge two flush_write_bio helpers
  btrfs: sink flush_fn to extent_write_cache_pages
  btrfs: sink writepage parameter to extent_write_cache_pages
  btrfs: unify extent_page_data type passed as void

 fs/btrfs/extent_io.c | 35 +--
 1 file changed, 13 insertions(+), 22 deletions(-)

-- 
2.15.1

--
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 0/9] Remaining part of mkfs --rootdir rework

[David Sterba]

On Wed, Nov 29, 2017 at 05:15:55PM +0800, Qu Wenruo wrote:
> Qu Wenruo (9):
>   btrfs-progs: mkfs: Cleanup temporary chunks before filling rootdir
>   btrfs-progs: mkfs: Don't use custom chunk allocator for rootdir
>   btrfs-progs: mkfs/rootdir: Use over-reserve method to make size
> estimate easier
>   btrfs-progs: mkfs/rootdir: Shrink fs for rootdir option
>   btrfs-progs: mkfs: Separate shrink from rootdir
>   btrfs-progs: mkfs: Fix regression preventing --rootdir to create file
>   btrfs-progs: tests/mkfs: Introduce test case to check if mkfs rootdir
> can create new file
>   btrfs-progs: mkfs: Use the whole file or block device to mkfs for
> rootdir
>   btrfs-progs: tests/mkfs: Introduce test case to verify if mkfs.btrfs
> rootdir shrink behaves correctly

Patches reviewed and merged. The basic test coverage is there, I'm CCing
Austin: we'd appreciate if you could also test it for your usecase.
Thanks.
--
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 1/2] btrfs-progs: convert: Fix a bug in rollback check which overwrite return value

[David Sterba]

On Wed, Nov 29, 2017 at 09:48:05PM +0800, Qu Wenruo wrote:
> Commit 1170ac307900 ("btrfs-progs: convert: Introduce function to check if
> convert image is able to be rolled back") reworked rollback check
> condition, by checking 1:1 mapping of each file extent.
> 
> The idea itself has nothing wrong, but error handler is not implemented
> correctly, which over writes the return value and always try to rollback
> the fs even it fails to pass the check.
> 
> Fix it by correctly return the error before rollback the fs.
> 
> Fixes: 1170ac307900 ("btrfs-progs: convert: Introduce function to check if 
> convert image is able to be rolled back")
> Reported-by: Jeff Mahoney 
> Signed-off-by: Qu Wenruo 

1-2 applied, thanks.
--
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 0/1] btrfs-progs: docs: annual typo, clarity, & grammar review & fixups

[David Sterba]

On Sat, Oct 21, 2017 at 08:00:45PM -0400, Nicholas D Steeves wrote:
> I've marked the parts the require review like this: This paragraph
> explains something but something is opaque (EDIT: reason why
> it's unclear or needs to be updated).  Here is what I found this
> year (@664dbbff6e72ffbdeb451b4f03f98dab3eabb1d1):

Thank you very much going through the docs. Patch applied to devel, I
went through all suggested changes, the questions you wrote inside ***
were helpful and the final text hopefully improved.
--
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 v4 72/73] xfs: Convert mru cache to XArray

[Dave Chinner]

On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote:
> On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote:
> > > Unfortunately for you, I don't find arguments along the lines of
> > > "lockdep will save us" at all convincing.  lockdep already throws
> > > too many false positives to be useful as a tool that reliably and
> > > accurately points out rare, exciting, complex, intricate locking
> > > problems.
> > 
> > But it does reliably and accurately point out "dude, you forgot to take
> > the lock".  It's caught a number of real problems in my own testing that
> > you never got to see.
> 
> The problem is that if it has too many false positives --- and it's
> gotten *way* worse with the completion callback "feature", people will
> just stop using Lockdep as being too annyoing and a waste of developer
> time when trying to figure what is a legitimate locking bug versus
> lockdep getting confused.
> 
> I can't even disable the new Lockdep feature which is throwing
> lots of new false positives --- it's just all or nothing.
> 
> Dave has just said he's already stopped using Lockdep, as a result.

This is compeltely OT, but FYI I stopped using lockdep a long time
ago.  We've spend orders of magnitude more time and effort to shut
up lockdep false positives in the XFS code than we ever have on
locking problems that lockdep has uncovered. And still lockdep
throws too many false positives on XFS workloads to be useful to me.

But it's more than that: I understand just how much lockdep *doesn't
check* and that means *I know I can't rely on lockdep* for potential
deadlock detection. e.g.  it doesn't cover semaphores, which means
it has zero coverage of the entire XFS metadata buffer subsystem and
the complex locking orders we have for metadata updates.

Put simply: lockdep doesn't provide me with any benefit, so I don't
use it...

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com
--
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

Lockdep is less useful than it was

[Matthew Wilcox]

On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote:
> The problem is that if it has too many false positives --- and it's
> gotten *way* worse with the completion callback "feature", people will
> just stop using Lockdep as being too annyoing and a waste of developer
> time when trying to figure what is a legitimate locking bug versus
> lockdep getting confused.
> 
> I can't even disable the new Lockdep feature which is throwing
> lots of new false positives --- it's just all or nothing.

You *can* ... but it's way more hacking Kconfig than you ought to have
to do (which is a separate rant ...)

You need to get LOCKDEP_CROSSRELEASE off.  I'd revert patches
e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and
b483cf3bc249d7af706390efa63d6671e80d1c09

I think it was a mistake to force these on for everybody; they have a
much higher false-positive rate than the rest of lockdep, so as you say
forcing them on leads to fewer people using *any* of lockdep.

The bug you're hitting isn't Byungchul's fault; it's an annotation
problem.  The same kind of annotation problem that we used to have with
dozens of other places in the kernel which are now fixed.  If you didn't
have to hack Kconfig to get rid of this problem, you'd be happier, right?
--
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: Lockdep is less useful than it was

[Matthew Wilcox]

On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote:
> You need to get LOCKDEP_CROSSRELEASE off.  I'd revert patches
> e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and
> b483cf3bc249d7af706390efa63d6671e80d1c09

Oops.  I meant to revert 2dcd5adfb7401b762ddbe4b86dcacc2f3de6b97b.
Or you could cherry-pick b483cf3bc249d7af706390efa63d6671e80d1c09.
--
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: Lockdep is less useful than it was

[Dave Chinner]

On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote:
> On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote:
> > The problem is that if it has too many false positives --- and it's
> > gotten *way* worse with the completion callback "feature", people will
> > just stop using Lockdep as being too annyoing and a waste of developer
> > time when trying to figure what is a legitimate locking bug versus
> > lockdep getting confused.
> > 
> > I can't even disable the new Lockdep feature which is throwing
> > lots of new false positives --- it's just all or nothing.
> 
> You *can* ... but it's way more hacking Kconfig than you ought to have
> to do (which is a separate rant ...)
> 
> You need to get LOCKDEP_CROSSRELEASE off.  I'd revert patches
> e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and
> b483cf3bc249d7af706390efa63d6671e80d1c09
> 
> I think it was a mistake to force these on for everybody; they have a
> much higher false-positive rate than the rest of lockdep, so as you say
> forcing them on leads to fewer people using *any* of lockdep.
> 
> The bug you're hitting isn't Byungchul's fault; it's an annotation
> problem.  The same kind of annotation problem that we used to have with
> dozens of other places in the kernel which are now fixed.

That's one of the fundamental problem with lockdep - it throws the
difficulty of solving all these new false positives onto the
developers who know nothing about lockdep and don't follow it's
development. And until they do solve them - especially in critical
subsystems that everyone uses like the storage stack - lockdep is
essentially worthless.

> If you didn't
> have to hack Kconfig to get rid of this problem, you'd be happier, right?

I'd be much happier if it wasn't turned on by default in the first
place.  We gave plenty of warnings that there were still unsolved
false positive problems with the new checks in the storage stack.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com
--
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 v5 0/3] Add cli and ioctl to deregister devices

[Anand Jain]

On 12/07/2017 10:52 PM, Austin S. Hemmelgarn wrote:

On 2017-12-07 09:36, Anand Jain wrote:

Add ability to deregister a or all devices. I have named this sub cmd
as deregister, but I am open to your suggestions.
Being a bit picky here, but from the perspective of a native speaker of 
American English, I would say that 'deregister' sounds rather synthetic 
and somewhat harsh and alien.


Given that, as odd as it sounds, I think 'ignore' might be a more user 
friendly name for the sub-command.  It accurately describes what the 
command is doing (telling the kernel to ignore the device), and it's a 
lot less alien sounding than 'deregister'.


If you're set on having it be based on the word 'register', I would 
suggest changing it to 'unregister', as I think that sounds more natural 
than 'deregister'.


Additionally, if you do continue with 'deregister' or go with 
'unregister' as the name though, I would suggest adding 'register' as a 
synonym for the 'scan' sub-command to keep things reasonably symmetrical.


 A look up on unregister lead me to use deregister as more appropriate.
 Anyway I won't bother much, I will be go be suggestions, and how
 about unscan, since scan is already there. OR how about purge.

Thanks, Anand.

--
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 v5 0/3] Add cli and ioctl to deregister devices

[Duncan]

Anand Jain posted on Fri, 08 Dec 2017 08:51:43 +0800 as excerpted:

> On 12/07/2017 10:52 PM, Austin S. Hemmelgarn wrote:
>> On 2017-12-07 09:36, Anand Jain wrote:
>>> Add ability to deregister a or all devices. I have named this sub cmd
>>> as deregister, but I am open to your suggestions.
>> Being a bit picky here, but from the perspective of a native speaker of
>> American English, I would say that 'deregister' sounds rather synthetic
>> and somewhat harsh and alien.
>> 
>> Given that, as odd as it sounds, I think 'ignore' might be a more user
>> friendly name for the sub-command.  It accurately describes what the
>> command is doing (telling the kernel to ignore the device), and it's a
>> lot less alien sounding than 'deregister'.
>> 
>> If you're set on having it be based on the word 'register', I would
>> suggest changing it to 'unregister', as I think that sounds more
>> natural than 'deregister'.
>> 
>> Additionally, if you do continue with 'deregister' or go with
>> 'unregister' as the name though, I would suggest adding 'register' as a
>> synonym for the 'scan' sub-command to keep things reasonably
>> symmetrical.
> 
>   A look up on unregister lead me to use deregister as more appropriate.
>   Anyway I won't bother much, I will be go be suggestions, and how about
>   unscan, since scan is already there. OR how about purge.

This is a bikeshed I think I have a beautiful color suggestion for! =:^)

Seriously, the normal purpose of scanning is to record particular details 
of what was seen during the scan, based on some desired scanning criteria.

So what do you call it when you need to "forget" the information you 
scanned for?

What about simply "forget", btrfs device forget ?  That sounds the most 
natural to me, certainly far more so than the apparently freshly created 
word "unscan", tho that would certainly deliver the meaning.

And FWIW, "deregister", just.. no.  (I too would vote unregister if we're 
sticking with the register root-word, but I have a feeling that may be a 
regional/en-US preference and some other English regional variants may 
find deregister is the less terrible to their ear.  That may explain 
whatever commentary under unregister led you to go with deregister.)  
"Deregister" sounds like something a computer programmer might say to 
describe the process, but I can't imagine a "normal" person using the 
word, except possibly in the context of removing someone from the voter 
rolls (where one "registers" to vote, so "deregister" could be a a 
reasonably natural term for reversing that) or the like.

-- 
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 v4 72/73] xfs: Convert mru cache to XArray

[Byungchul Park]

On Fri, Dec 08, 2017 at 09:22:16AM +1100, Dave Chinner wrote:
> On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote:
> > On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote:
> > > > Unfortunately for you, I don't find arguments along the lines of
> > > > "lockdep will save us" at all convincing.  lockdep already throws
> > > > too many false positives to be useful as a tool that reliably and
> > > > accurately points out rare, exciting, complex, intricate locking
> > > > problems.
> > > 
> > > But it does reliably and accurately point out "dude, you forgot to take
> > > the lock".  It's caught a number of real problems in my own testing that
> > > you never got to see.
> > 
> > The problem is that if it has too many false positives --- and it's
> > gotten *way* worse with the completion callback "feature", people will
> > just stop using Lockdep as being too annyoing and a waste of developer
> > time when trying to figure what is a legitimate locking bug versus
> > lockdep getting confused.
> > 
> > I can't even disable the new Lockdep feature which is throwing
> > lots of new false positives --- it's just all or nothing.
> > 
> > Dave has just said he's already stopped using Lockdep, as a result.
> 
> This is compeltely OT, but FYI I stopped using lockdep a long time
> ago.  We've spend orders of magnitude more time and effort to shut
> up lockdep false positives in the XFS code than we ever have on
> locking problems that lockdep has uncovered. And still lockdep
> throws too many false positives on XFS workloads to be useful to me.
> 
> But it's more than that: I understand just how much lockdep *doesn't
> check* and that means *I know I can't rely on lockdep* for potential
> deadlock detection. e.g.  it doesn't cover semaphores, which means

Hello,

I'm careful in saying the following since you seem to feel not good at
crossrelease and even lockdep. Now that cross-release has been
introduced, semaphores can be covered as you might know. Actually, all
general waiters can.

> it has zero coverage of the entire XFS metadata buffer subsystem and
> the complex locking orders we have for metadata updates.
> 
> Put simply: lockdep doesn't provide me with any benefit, so I don't
> use it...

Sad..

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> da...@fromorbit.com
--
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] Btrfs : send, truncate first to enhance many small files

[robbieko]

Hi Filipe David Manana,

I'm sorry to reply so late, your patch is good for me.
Will you submit this patch to the upstream?
In addition, you mentioned other optimization, Can you share it?

I have another case, find_extent_clone will be too slow when there is 
too much backref in the big file.
1G file with fallocate, have 4 extent_item, then do some 4k random write 
IO, finally total 76767 EXTENT_DATA.

My test ignored find_extent_clone, much faster.
original: 16m8.387s
ignore find_extent_clone: 12s
Do you have a better solution?

Thanks.
Robbie Ko

Filipe Manana 於 2017-12-04 20:08 寫到:
On Mon, Dec 4, 2017 at 11:28 AM, Qu Wenruo  
wrote:



On 2017年12月04日 19:13, Filipe Manana wrote:
On Mon, Dec 4, 2017 at 7:19 AM, Qu Wenruo  
wrote:



On 2017年12月04日 15:02, robbieko wrote:

From: Robbie Ko 

The commands generated by send contain the following step:
1. mkfile o1851-19-0
2. rename o1851-19-0 -> alsa-driver/alsa-kernel/isa/es1688/es1688.c
3. set_xattr alsa-driver/alsa-kernel/isa/es1688/es1688.c - 
name=user.xattr data_len=4 data=test
4. write alsa-driver/alsa-kernel/isa/es1688/es1688.c - offset=0, 
len=10458

5. truncate alsa-driver/alsa-kernel/isa/es1688/es1688.c size=10458
6. chown alsa-driver/alsa-kernel/isa/es1688/es1688.c - uid=1024, 
gid=100

7. chmod alsa-driver/alsa-kernel/isa/es1688/es1688.c - mode=0644
8. utimes alsa-driver/alsa-kernel/isa/es1688/es1688.c

After writing file content, it will truncate file to the correct 
size.
Btrfs truncate will flush last page if size does not align to 
sectorsize,

and this will cause receive process to wait until flush finishes.
In order to avoid waiting flushing data.This patch changes the 
order so

that truncate command is sent before write command.


Personally speaking, it's better to optimize the receive side.

For example, at receive side, if we already know that the file size 
is

not changed at all, then just skip the truncate command.

In your send dump, step 5 is not needed at all, and can be skipped 
to

speed up the receive procedure.


Better yet, is to not send the truncate commands at all, reduces the
stream size and saves all receiver implementations
from having such logic (which further requires stat calls to figure
out the current file size).


Another one is to optimize btrfs file truncate to avoid the 
unnecessary

page writeback.

But at least, doing optimization in receive side is easier and less
bug-prone.


Not really, for this case at least.


If we could do the same in both send and receive side, then I prefer
optimization in receive side.


Can't agree much with that, for the reasons mentioned before.



Thanks,
Qu



I still have a local branch around with several optimizations (from
the time I was a volunteer), with one of them being precisely to 
avoid
unnecessary truncate commands, however I never ended up getting to 
the

point of doing benchmarks.
Pasted below and at 
https://www.friendpaste.com/1J1TuXf2wlU1125mX57k4r



commit 362290ea03b52e4ce4edb43453a51a3b1f1cd33f
Author: Filipe David Borba Manana 
Date:   Sat Apr 5 22:34:45 2014 +0100

Btrfs: send, don't issue unnecessary file truncate commands

Every time a file is created or updated, send (either incremental
or not) always
issues a file truncate command. In several commons scenarios this
truncate is not
needed and yet it was still issued all the time. The unnecessary
cases where the
truncate command is not needed are:

* We have a new file, and its last extent isn't a hole, so that
its last write
  command ends at the file's size;

* For an incremental send, we updated in place an existing file,
without changing
  its size;

* For an incremental send, we updated the file, increased its
size, and the last
  file extent item doesn't correspond to a hole. In this case the 
last write

  command against the file ends at the file's new size;

* For an incremental send, we didn't update the file's data at
all, we only changed
  its mode, group or access/update timestamps.

Therefore don't send truncate commands for these cases, reducing
the stream's size
and saving time.

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index fa378c7ebffd..46f52b4bbc21 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -120,6 +120,7 @@ struct send_ctx {
  u64 cur_inode_mode;
  u64 cur_inode_rdev;
  u64 cur_inode_last_extent;
+ u64 cur_inode_max_write_end;

  u64 send_progress;
  u64 total_data_size;
@@ -4494,6 +4495,8 @@ static int send_hole(struct send_ctx *sctx, u64 
end)

  break;
  offset += len;
  }
+ sctx->cur_inode_max_write_end = max(offset,
+sctx->cur_inode_max_write_end);
 tlv_put_failure:
  fs_path_free(p);
  return ret;
@@ -4529,6 +4532,10 @@ static int send_write_or_clone(struct send_ctx 
*sctx,

  len = btrfs_file_extent_num_bytes(path->nodes[0], ei);
  }

+ if (offset >= sctx->cur_inode_size) {
+ ret = 0;
+ goto out;
+ }
  if (offset + len > sctx->cur_inode_size)
  len = sctx->cur_inode_size - offset;
  if (len

Re: [PATCH] Btrfs : send, truncate first to enhance many small files

[Qu Wenruo]

On 2017年12月08日 14:29, robbieko wrote:
> Hi Filipe David Manana,
> 
> I'm sorry to reply so late, your patch is good for me.
> Will you submit this patch to the upstream?
> In addition, you mentioned other optimization, Can you share it?
> 
> I have another case, find_extent_clone will be too slow when there is
> too much backref in the big file.

I submitted an RFC patch long time ago.
https://patchwork.kernel.org/patch/9245287/

That will break reflink and got objected.

> 1G file with fallocate, have 4 extent_item, then do some 4k random write
> IO, finally total 76767 EXTENT_DATA.

We already have more dangerous test case for that.

Check fstests/btrfs/130, which can even lead to OOM on small memory system.

Thanks,
Qu

> My test ignored find_extent_clone, much faster.
> original: 16m8.387s
> ignore find_extent_clone: 12s
> Do you have a better solution?
> 
> Thanks.
> Robbie Ko
> 
> Filipe Manana 於 2017-12-04 20:08 寫到:
>> On Mon, Dec 4, 2017 at 11:28 AM, Qu Wenruo 
>> wrote:
>>>
>>>
>>> On 2017年12月04日 19:13, Filipe Manana wrote:
 On Mon, Dec 4, 2017 at 7:19 AM, Qu Wenruo 
 wrote:
>
>
> On 2017年12月04日 15:02, robbieko wrote:
>> From: Robbie Ko 
>>
>> The commands generated by send contain the following step:
>> 1. mkfile o1851-19-0
>> 2. rename o1851-19-0 -> alsa-driver/alsa-kernel/isa/es1688/es1688.c
>> 3. set_xattr alsa-driver/alsa-kernel/isa/es1688/es1688.c -
>> name=user.xattr data_len=4 data=test
>> 4. write alsa-driver/alsa-kernel/isa/es1688/es1688.c - offset=0,
>> len=10458
>> 5. truncate alsa-driver/alsa-kernel/isa/es1688/es1688.c size=10458
>> 6. chown alsa-driver/alsa-kernel/isa/es1688/es1688.c - uid=1024,
>> gid=100
>> 7. chmod alsa-driver/alsa-kernel/isa/es1688/es1688.c - mode=0644
>> 8. utimes alsa-driver/alsa-kernel/isa/es1688/es1688.c
>>
>> After writing file content, it will truncate file to the correct
>> size.
>> Btrfs truncate will flush last page if size does not align to
>> sectorsize,
>> and this will cause receive process to wait until flush finishes.
>> In order to avoid waiting flushing data.This patch changes the
>> order so
>> that truncate command is sent before write command.
>
> Personally speaking, it's better to optimize the receive side.
>
> For example, at receive side, if we already know that the file size is
> not changed at all, then just skip the truncate command.
>
> In your send dump, step 5 is not needed at all, and can be skipped to
> speed up the receive procedure.

 Better yet, is to not send the truncate commands at all, reduces the
 stream size and saves all receiver implementations
 from having such logic (which further requires stat calls to figure
 out the current file size).
>>>
>>> Another one is to optimize btrfs file truncate to avoid the unnecessary
>>> page writeback.
>>>
>>> But at least, doing optimization in receive side is easier and less
>>> bug-prone.
>>
>> Not really, for this case at least.
>>>
>>> If we could do the same in both send and receive side, then I prefer
>>> optimization in receive side.
>>
>> Can't agree much with that, for the reasons mentioned before.
>>
>>>
>>> Thanks,
>>> Qu
>>>

 I still have a local branch around with several optimizations (from
 the time I was a volunteer), with one of them being precisely to avoid
 unnecessary truncate commands, however I never ended up getting to the
 point of doing benchmarks.
 Pasted below and at https://www.friendpaste.com/1J1TuXf2wlU1125mX57k4r


 commit 362290ea03b52e4ce4edb43453a51a3b1f1cd33f
 Author: Filipe David Borba Manana 
 Date:   Sat Apr 5 22:34:45 2014 +0100

     Btrfs: send, don't issue unnecessary file truncate commands

     Every time a file is created or updated, send (either incremental
 or not) always
     issues a file truncate command. In several commons scenarios this
 truncate is not
     needed and yet it was still issued all the time. The unnecessary
 cases where the
     truncate command is not needed are:

     * We have a new file, and its last extent isn't a hole, so that
 its last write
   command ends at the file's size;

     * For an incremental send, we updated in place an existing file,
 without changing
   its size;

     * For an incremental send, we updated the file, increased its
 size, and the last
   file extent item doesn't correspond to a hole. In this case
 the last write
   command against the file ends at the file's new size;

     * For an incremental send, we didn't update the file's data at
 all, we only changed
   its mode, group or access/update timestamps.

     Therefore don't send truncate commands for these cases, reducing
 the stream's size
     and saving time.

 diff -

Re: [PATCH v4 72/73] xfs: Convert mru cache to XArray

[Dave Chinner]

On Fri, Dec 08, 2017 at 01:45:52PM +0900, Byungchul Park wrote:
> On Fri, Dec 08, 2017 at 09:22:16AM +1100, Dave Chinner wrote:
> > On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote:
> > > On Wed, Dec 06, 2017 at 06:06:48AM -0800, Matthew Wilcox wrote:
> > > > > Unfortunately for you, I don't find arguments along the lines of
> > > > > "lockdep will save us" at all convincing.  lockdep already throws
> > > > > too many false positives to be useful as a tool that reliably and
> > > > > accurately points out rare, exciting, complex, intricate locking
> > > > > problems.
> > > > 
> > > > But it does reliably and accurately point out "dude, you forgot to take
> > > > the lock".  It's caught a number of real problems in my own testing that
> > > > you never got to see.
> > > 
> > > The problem is that if it has too many false positives --- and it's
> > > gotten *way* worse with the completion callback "feature", people will
> > > just stop using Lockdep as being too annyoing and a waste of developer
> > > time when trying to figure what is a legitimate locking bug versus
> > > lockdep getting confused.
> > > 
> > > I can't even disable the new Lockdep feature which is throwing
> > > lots of new false positives --- it's just all or nothing.
> > > 
> > > Dave has just said he's already stopped using Lockdep, as a result.
> > 
> > This is compeltely OT, but FYI I stopped using lockdep a long time
> > ago.  We've spend orders of magnitude more time and effort to shut
> > up lockdep false positives in the XFS code than we ever have on
> > locking problems that lockdep has uncovered. And still lockdep
> > throws too many false positives on XFS workloads to be useful to me.
> > 
> > But it's more than that: I understand just how much lockdep *doesn't
> > check* and that means *I know I can't rely on lockdep* for potential
> > deadlock detection. e.g.  it doesn't cover semaphores, which means
> 
> Hello,
> 
> I'm careful in saying the following since you seem to feel not good at
> crossrelease and even lockdep. Now that cross-release has been
> introduced, semaphores can be covered as you might know. Actually, all
> general waiters can.

And all it will do is create a whole bunch more work for us XFS guys
to shut up all the the false positive crap that falls out from it
because the locking model we have is far more complex than any of
the lockdep developers thought was necessary to support, just like
happened with the XFS inode annotations all those years ago.

e.g. nobody has ever bothered to ask us what is needed to describe
XFS's semaphore locking model.  If you did that, you'd know that we
nest *thousands* of locked semaphores in compeltely random lock
order during metadata buffer writeback. And that this lock order
does not reflect the actual locking order rules we have for locking
buffers during transactions.

Oh, and you'd also know that a semaphore's lock order and context
can change multiple times during the life time of the buffer.  Say
we free a block and the reallocate it as something else before it is
reclaimed - that buffer now might have a different lock order. Or
maybe we promote a buffer to be a root btree block as a result of a
join - it's now the first buffer in a lock run, rather than a child.
Or we split a tree, and the root is now a node and so no longer is
the first buffer in a lock run. Or that we walk sideways along the
leaf nodes siblings during searches.  IOWs, there is no well defined
static lock ordering at all for buffers - and therefore semaphores -
in XFS at all.

And knowing that, you wouldn't simply mention that lockdep can
support semaphores now as though that is necessary to "make it work"
for XFS.  It's going to be much simpler for us to just turn off
lockdep and ignore whatever crap it sends our way than it is to
spend unplanned weeks of our time to try to make lockdep sorta work
again. Sure, we might get there in the end, but it's likely to take
months, if not years like it did with the XFS inode annotations.

> > it has zero coverage of the entire XFS metadata buffer subsystem and
> > the complex locking orders we have for metadata updates.
> > 
> > Put simply: lockdep doesn't provide me with any benefit, so I don't
> > use it...
> 
> Sad..

I don't think you understand. I'll try to explain.

The lockdep infrastructure by itself doesn't make lockdep a useful
tool - it mostly generates false positives because it has no
concept of locking models that don't match it's internal tracking
assumptions and/or limitations.

That means if we can't suppress the false positives, then lockdep is
going to be too noisy to find real problems.  It's taken the XFS
developers months of work over the past 7-8 years to suppress all
the *common* false positives that lockdep throws on XFS. And despite
all that work, there's still too many false positives occuring
because we can't easily suppress them with annotations. IOWs, the
signal to noise ratio is still too low for lockdep to find 

[PATCH RFC] btrfs: self heal from SB fail

[Anand Jain]

-EXPERIMENTAL-
As of now when primary SB fails we won't self heal and would fail mount,
this is an experimental patch which thinks why not go and read backup
copy.

Signed-off-by: Anand Jain 
---
 fs/btrfs/disk-io.c |  8 +++-
 fs/btrfs/volumes.c | 10 +++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9b20c1f3563b..a791b8dfe8a8 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3190,7 +3190,7 @@ struct buffer_head *btrfs_read_dev_super(struct 
block_device *bdev)
 * So, we need to add a special mount option to scan for
 * later supers, using BTRFS_SUPER_MIRROR_MAX instead
 */
-   for (i = 0; i < 1; i++) {
+   for (i = 0; i < BTRFS_SUPER_MIRROR_MAX; i++) {
ret = btrfs_read_dev_one_super(bdev, i, &bh);
if (ret)
continue;
@@ -4015,11 +4015,17 @@ static int btrfs_check_super_valid(struct btrfs_fs_info 
*fs_info)
ret = -EINVAL;
}
 
+#if 0
+   /*
+* Need a way to check for any copy of SB, as its not a
+* strong check, just ignore this for now.
+*/
if (btrfs_super_bytenr(sb) != BTRFS_SUPER_INFO_OFFSET) {
btrfs_err(fs_info, "super offset mismatch %llu != %u",
  btrfs_super_bytenr(sb), BTRFS_SUPER_INFO_OFFSET);
ret = -EINVAL;
}
+#endif
 
/*
 * Obvious sys_chunk_array corruptions, it must hold at least one key
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9fa2539a8493..f368db94d62b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1369,7 +1369,7 @@ int btrfs_scan_one_device(const char *path, fmode_t 
flags, void *holder,
 {
struct btrfs_super_block *disk_super;
struct block_device *bdev;
-   struct page *page;
+   struct buffer_head *sb_bh;
int ret = -EINVAL;
u64 devid;
u64 transid;
@@ -1392,8 +1392,12 @@ int btrfs_scan_one_device(const char *path, fmode_t 
flags, void *holder,
goto error;
}
 
-   if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super))
+   sb_bh = btrfs_read_dev_super(bdev);
+   if (IS_ERR(sb_bh)) {
+   ret = PTR_ERR(sb_bh);
goto error_bdev_put;
+   }
+   disk_super = (struct btrfs_super_block *) sb_bh->b_data;
 
devid = btrfs_stack_device_id(&disk_super->dev_item);
transid = btrfs_super_generation(disk_super);
@@ -1413,7 +1417,7 @@ int btrfs_scan_one_device(const char *path, fmode_t 
flags, void *holder,
if (!ret && fs_devices_ret)
(*fs_devices_ret)->total_devices = total_devices;
 
-   btrfs_release_disk_super(page);
+   brelse(sb_bh);
 
 error_bdev_put:
blkdev_put(bdev, flags);
-- 
2.7.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