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