Re: [PATCH] btrfs: use list_for_each_entry_safe() when delete items
On Sat, Jul 27, 2013 at 2:12 PM, Azat Khuzhin wrote: > Replace list_for_each_entry() by list_for_each_entry_safe() in > __btrfs_close_devices() > > There is another place that delete items lock_stripe_add(), but there we > don't need safe version, because after deleting we exit from loop. > > Signed-off-by: Azat Khuzhin > --- > fs/btrfs/volumes.c |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 78b8717..1d1b595 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -616,13 +616,13 @@ static void free_device(struct rcu_head *head) > > static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices) > { > - struct btrfs_device *device; > + struct btrfs_device *device, *next; > > if (--fs_devices->opened > 0) > return 0; > > mutex_lock(&fs_devices->device_list_mutex); > - list_for_each_entry(device, &fs_devices->devices, dev_list) { > + list_for_each_entry_safe(device, next, &fs_devices->devices, > dev_list) { > struct btrfs_device *new_device; > struct rcu_string *name; There is "kfree(device);" at the end of loop, maybe there must "goto again;" after it? (instead of this patch) > > -- > 1.7.10.4 > -- Respectfully Azat Khuzhin -- 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: btrfs quota examples?
On Mon, 10 Jun 2013 09:41:39 +0200 Arne Jansen wrote: > > Now, my questions: > > > > - what do both 104882176 104882176 numbers represent? > > The first number represents the amount of data in that subvolume, > regardless whether that data is shared with other subvolumes or > not. > The second number shows the amount of data that is unique to this > subvolume and not shared with others, i.e. the amount of space > that will get freed if you delete this subvolume. I've played with qgroups for some time, but the results are rather inconsistent. I.e. here - what does a negative number represent in 0/1181 row? # btrfs qgroup show /mnt/lxc2 0/260 151490953216 151490953216 0/261 180969472 180969472 0/262 17888 983040 0/377 180310016 25776128 0/378 304088072192 304088072192 0/535 571944960 417370112 0/536 68550987776 68550987776 0/642 247463936 92921856 0/1175 617213952 827392 0/1181 16112013312 -22184235008 0/1268 38296248320 0 0/1269 616386560 0 0/1270 4096 4096 0/1271 4096 4096 -- Tomasz Chmielewski http://wpkg.org -- 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: Fwd: Cloning a Btrfs partition
Hi BJ, [original message rewrapped] On Thu, July 25, 2013 at 18:32 (+0200), BJ Quinn wrote: > (Apologies for the double post -- forgot to send as plain text the first time > around, so the list rejected it.) > > I see that there's now a btrfs send / receive and I've tried using it, but > I'm getting the oops I've pasted below, after which the FS becomes > unresponsive (no I/O to the drive, no CPU usage, but all attempts to access > the FS results in a hang). I have an internal drive (single drive) that > contains 82GB of compressed data with a couple hundred snapshots. I tried > taking the first snapshot and making a read only copy (btrfs subvolume > snapshot -r) and then I connected an external USB drive and ran btrfs send / > receive to that external drive. It starts working and gets a couple of GB in > (I'd expect the first snapshot to be about 20GB) and then gets the following > error. I had to use the latest copy of btrfs-progs from git, because the > package installed on my system (btrfs-progs-0.20-0.2.git91d9eec) simply > returned "invalid argument" when trying to run btrfs send / receive. Thanks > in advance for any info you may have. The problem has been introduced with rbtree ulists in 3.10, commit Btrfs: add a rb_tree to improve performance of ulist search You should be safe to revert that commit, it's a performance optimization attempt. Alternatively, you can apply the published fix Btrfs: fix crash regarding to ulist_add_merge It has not made it into 3.10 stable or 3.11, yet, but is contained in Josef's btrfs-next git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git Thanks, -Jan -- 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 v3] Btrfs: fix crash regarding to ulist_add_merge
On Fri, June 28, 2013 at 06:37 (+0200), Liu Bo wrote: > Several users reported this crash of NULL pointer or general protection, > the story is that we add a rbtree for speedup ulist iteration, and we > use krealloc() to address ulist growth, and krealloc() use memcpy to copy > old data to new memory area, so it's OK for an array as it doesn't use > pointers while it's not OK for a rbtree as it uses pointers. > > So krealloc() will mess up our rbtree and it ends up with crash. > > Reviewed-by: Wang Shilong > Signed-off-by: Liu Bo > --- > v3: fix a return value problem(Thanks Wang Shilong). > v2: fix an use-after-free bug and a finger error(Thanks Zach and Josef). > > fs/btrfs/ulist.c | 15 +++ > 1 files changed, 15 insertions(+), 0 deletions(-) > > diff --git a/fs/btrfs/ulist.c b/fs/btrfs/ulist.c > index 7b417e2..b0a523b2 100644 > --- a/fs/btrfs/ulist.c > +++ b/fs/btrfs/ulist.c > @@ -205,6 +205,10 @@ int ulist_add_merge(struct ulist *ulist, u64 val, u64 > aux, > u64 new_alloced = ulist->nodes_alloced + 128; > struct ulist_node *new_nodes; > void *old = NULL; > + int i; > + > + for (i = 0; i < ulist->nnodes; i++) > + rb_erase(&ulist->nodes[i].rb_node, &ulist->root); > > /* >* if nodes_alloced == ULIST_SIZE no memory has been allocated > @@ -224,6 +228,17 @@ int ulist_add_merge(struct ulist *ulist, u64 val, u64 > aux, > > ulist->nodes = new_nodes; > ulist->nodes_alloced = new_alloced; > + > + /* > + * krealloc actually uses memcpy, which does not copy rb_node > + * pointers, so we have to do it ourselves. Otherwise we may > + * be bitten by crashes. > + */ > + for (i = 0; i < ulist->nnodes; i++) { > + ret = ulist_rbtree_insert(ulist, &ulist->nodes[i]); > + if (ret < 0) > + return ret; > + } > } > ulist->nodes[ulist->nnodes].val = val; > ulist->nodes[ulist->nnodes].aux = aux; > Reviewed-by: Jan Schmidt Josef, how about sending this one for the next 3.11 rc and to 3.10 stable? Any objections? -Jan -- 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: Recovering from btrfs error Couldn't read chunk root.
在 2013-7-29,上午2:12,Kyriakos 写道: > Just tried it as you said with the -v option enabled > This is my output: > > http://bpaste.net/show/118112/ > > This is a *long* email, and seems that btrfs list refuse it. > Device extent: devid = 1, start = 1667558801408, len = 1073741824, > chunk offset = 1663255445504 > Couldn't map the block 626309926912 > btrfs: volumes.c:1020: btrfs_num_copies: Assertion `!(ce->start > > logical || ce->start + ce->size < logical)' failed. > Aborted (core dumped) Strange enough, we don't find any chunks during scanning process. And seems this is unrecoverable ~_~ Wang, > > > Any thoughts? > > On 28 July 2013 08:17, Wang Shilong wrote: >> Hello, >> >> It seems Btrfs Chunk Tree is damaged, so you can not mount Btrfs filesystem >> any more. >> >> However, you can try the latest Btrfs-progs, Miao Xie implements chunk tree >> recover function. >> >> The url is: >> >>git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs.git >> >> >> you can try it: >>btrfs chunk-recover -v >> >> This is Time-consuming, because it will scan the whole disk. And also, >> please catch output of processing(this is helpful to us if the recovery >> fails, -v option >> enable this). >> >> Thanks, >> Wang >> >> >> >> >> >> -- 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 v3] Btrfs: fix crash regarding to ulist_add_merge
On Mon, Jul 29, 2013 at 10:23:34AM +0200, Jan Schmidt wrote: > On Fri, June 28, 2013 at 06:37 (+0200), Liu Bo wrote: > > Several users reported this crash of NULL pointer or general protection, > > the story is that we add a rbtree for speedup ulist iteration, and we > > use krealloc() to address ulist growth, and krealloc() use memcpy to copy > > old data to new memory area, so it's OK for an array as it doesn't use > > pointers while it's not OK for a rbtree as it uses pointers. > > > > So krealloc() will mess up our rbtree and it ends up with crash. > > > > Reviewed-by: Wang Shilong > > Signed-off-by: Liu Bo > > --- > > v3: fix a return value problem(Thanks Wang Shilong). > > v2: fix an use-after-free bug and a finger error(Thanks Zach and Josef). > > > > fs/btrfs/ulist.c | 15 +++ > > 1 files changed, 15 insertions(+), 0 deletions(-) > > > > diff --git a/fs/btrfs/ulist.c b/fs/btrfs/ulist.c > > index 7b417e2..b0a523b2 100644 > > --- a/fs/btrfs/ulist.c > > +++ b/fs/btrfs/ulist.c > > @@ -205,6 +205,10 @@ int ulist_add_merge(struct ulist *ulist, u64 val, u64 > > aux, > > u64 new_alloced = ulist->nodes_alloced + 128; > > struct ulist_node *new_nodes; > > void *old = NULL; > > + int i; > > + > > + for (i = 0; i < ulist->nnodes; i++) > > + rb_erase(&ulist->nodes[i].rb_node, &ulist->root); > > > > /* > > * if nodes_alloced == ULIST_SIZE no memory has been allocated > > @@ -224,6 +228,17 @@ int ulist_add_merge(struct ulist *ulist, u64 val, u64 > > aux, > > > > ulist->nodes = new_nodes; > > ulist->nodes_alloced = new_alloced; > > + > > + /* > > +* krealloc actually uses memcpy, which does not copy rb_node > > +* pointers, so we have to do it ourselves. Otherwise we may > > +* be bitten by crashes. > > +*/ > > + for (i = 0; i < ulist->nnodes; i++) { > > + ret = ulist_rbtree_insert(ulist, &ulist->nodes[i]); > > + if (ret < 0) > > + return ret; > > + } > > } > > ulist->nodes[ulist->nnodes].val = val; > > ulist->nodes[ulist->nnodes].aux = aux; > > > > Reviewed-by: Jan Schmidt > > Josef, how about sending this one for the next 3.11 rc and to 3.10 stable? Any > objections? A good candidate for -stable, along with josef's 'last ref' fixes about __tree_mod_log_rewind(), thanks, -liubo -- 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] Btrfs: cleanup arguments to extent_clear_unlock_delalloc
This patch removes the io_tree argument for extent_clear_unlock_delalloc since we always use &BTRFS_I(inode)->io_tree, and it separates out the extent tree operations from the page operations. This way we just pass in the extent bits we want to clear and then pass in the operations we want done to the pages. This is because I'm going to fix what extent bits we clear in some cases and rather than add a bunch of new flags we'll just use the actual extent bits we want to clear. Thanks, Signed-off-by: Josef Bacik --- fs/btrfs/extent_io.c | 32 --- fs/btrfs/extent_io.h | 21 +++ fs/btrfs/inode.c | 162 -- 3 files changed, 85 insertions(+), 130 deletions(-) diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index cc685dd..deaea9c 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -1678,31 +1678,21 @@ out_failed: return found; } -int extent_clear_unlock_delalloc(struct inode *inode, - struct extent_io_tree *tree, - u64 start, u64 end, struct page *locked_page, - unsigned long op) +int extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end, +struct page *locked_page, +unsigned long clear_bits, +unsigned long page_ops) { + struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree; int ret; struct page *pages[16]; unsigned long index = start >> PAGE_CACHE_SHIFT; unsigned long end_index = end >> PAGE_CACHE_SHIFT; unsigned long nr_pages = end_index - index + 1; int i; - unsigned long clear_bits = 0; - - if (op & EXTENT_CLEAR_UNLOCK) - clear_bits |= EXTENT_LOCKED; - if (op & EXTENT_CLEAR_DIRTY) - clear_bits |= EXTENT_DIRTY; - - if (op & EXTENT_CLEAR_DELALLOC) - clear_bits |= EXTENT_DELALLOC; clear_extent_bit(tree, start, end, clear_bits, 1, 0, NULL, GFP_NOFS); - if (!(op & (EXTENT_CLEAR_UNLOCK_PAGE | EXTENT_CLEAR_DIRTY | - EXTENT_SET_WRITEBACK | EXTENT_END_WRITEBACK | - EXTENT_SET_PRIVATE2))) + if (page_ops == 0) return 0; while (nr_pages > 0) { @@ -1711,20 +1701,20 @@ int extent_clear_unlock_delalloc(struct inode *inode, nr_pages, ARRAY_SIZE(pages)), pages); for (i = 0; i < ret; i++) { - if (op & EXTENT_SET_PRIVATE2) + if (page_ops & PAGE_SET_PRIVATE2) SetPagePrivate2(pages[i]); if (pages[i] == locked_page) { page_cache_release(pages[i]); continue; } - if (op & EXTENT_CLEAR_DIRTY) + if (page_ops & PAGE_CLEAR_DIRTY) clear_page_dirty_for_io(pages[i]); - if (op & EXTENT_SET_WRITEBACK) + if (page_ops & PAGE_SET_WRITEBACK) set_page_writeback(pages[i]); - if (op & EXTENT_END_WRITEBACK) + if (page_ops & PAGE_END_WRITEBACK) end_page_writeback(pages[i]); - if (op & EXTENT_CLEAR_UNLOCK_PAGE) + if (page_ops & PAGE_UNLOCK) unlock_page(pages[i]); page_cache_release(pages[i]); } diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h index f7544af..c450620 100644 --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -44,14 +44,11 @@ #define EXTENT_BUFFER_DUMMY 9 /* these are flags for extent_clear_unlock_delalloc */ -#define EXTENT_CLEAR_UNLOCK_PAGE 0x1 -#define EXTENT_CLEAR_UNLOCK 0x2 -#define EXTENT_CLEAR_DELALLOC 0x4 -#define EXTENT_CLEAR_DIRTY 0x8 -#define EXTENT_SET_WRITEBACK0x10 -#define EXTENT_END_WRITEBACK0x20 -#define EXTENT_SET_PRIVATE2 0x40 -#define EXTENT_CLEAR_ACCOUNTING 0x80 +#define PAGE_UNLOCK(1 << 0) +#define PAGE_CLEAR_DIRTY (1 << 1) +#define PAGE_SET_WRITEBACK (1 << 2) +#define PAGE_END_WRITEBACK (1 << 3) +#define PAGE_SET_PRIVATE2 (1 << 4) /* * page->private values. Every page that is controlled by the extent @@ -328,10 +325,10 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long offset, unsigned long *map_len); int extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end); int extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end); -int extent_clear_unlock_delalloc(struct inode *inode, - struct extent_io_tree *tree, - u64 start, u64 end,
Re: Cloning a Btrfs partition
Thanks for the response! Not sure I want to roll a custom kernel on this particular system. Any idea on when it might make it to 3.10 stable or 3.11? Or should I just revert back to 3.9? Thanks! -BJ - Original Message - From: "Jan Schmidt" Sent: Monday, July 29, 2013 3:21:51 AM Hi BJ, [original message rewrapped] On Thu, July 25, 2013 at 18:32 (+0200), BJ Quinn wrote: > (Apologies for the double post -- forgot to send as plain text the first time > around, so the list rejected it.) > > I see that there's now a btrfs send / receive and I've tried using it, but > I'm getting the oops I've pasted below, after which the FS becomes > unresponsive (no I/O to the drive, no CPU usage, but all attempts to access > the FS results in a hang). I have an internal drive (single drive) that > contains 82GB of compressed data with a couple hundred snapshots. I tried > taking the first snapshot and making a read only copy (btrfs subvolume > snapshot -r) and then I connected an external USB drive and ran btrfs send / > receive to that external drive. It starts working and gets a couple of GB in > (I'd expect the first snapshot to be about 20GB) and then gets the following > error. I had to use the latest copy of btrfs-progs from git, because the > package installed on my system (btrfs-progs-0.20-0.2.git91d9eec) simply > returned "invalid argument" when trying to run btrfs send / receive. Thanks > in advance for any info you may have. The problem has been introduced with rbtree ulists in 3.10, commit Btrfs: add a rb_tree to improve performance of ulist search You should be safe to revert that commit, it's a performance optimization attempt. Alternatively, you can apply the published fix Btrfs: fix crash regarding to ulist_add_merge It has not made it into 3.10 stable or 3.11, yet, but is contained in Josef's btrfs-next git://git.kernel.org/pub/scm/linux/kernel/git/josef/btrfs-next.git Thanks, -Jan -- 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
Online data deduplication
Hello, I am willing to perform QA on online data deduplication. From where can i download the patches? -- Thanks, Hemanth Kumar H C -- 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] xfstests: generic/315: add one more sync and more output
So df in btrfs is tricky at best, and relying on it for accurate information is not great, but it's the best way to verify this test. So we need to add another sync to make sure the pinned blocks are all freed up and the df space is really really accurate, otherwise we end up with this test failling because the df after the test is slightly off (in my case it was like 36kb off). With this patch I'm not seeing random failures of this test. Thanks, Signed-off-by: Josef Bacik --- tests/generic/315 |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tests/generic/315 b/tests/generic/315 index 7cfc40d..7c55b8a 100644 --- a/tests/generic/315 +++ b/tests/generic/315 @@ -70,10 +70,11 @@ fsize=`ls -l $TEST_DIR/testfile.$seq | awk '{print $5}'` # Truncate the file size back to 0 truncate -s 0 $TEST_DIR/testfile.$seq sync +sync # Preallocated disk space should be released avail_done=`df -P $TEST_DIR | awk 'END {print $4}'` -[ "$avail_done" -eq "$avail_begin" ] || _fail "Available disk space ($avail_done KiB)" +[ "$avail_done" -eq "$avail_begin" ] || _fail "Available disk space ($avail_done KiB) wanted ($avail_begin KiB)" # success, all done exit -- 1.7.7.6 -- 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] Btrfs: fix what bits we clear when erroring out from delalloc
First of all we no longer set EXTENT_DIRTY when we dirty an extent so this patch removes the clearing of EXTENT_DIRTY since it confuses me. This patch also adds clearing EXTENT_DEFRAG and also doing EXTENT_DO_ACCOUNTING when we have errors. This is because if we are clearing delalloc without adding an ordered extent then we need to make sure the enospc handling stuff is accounted for. Also if this range was DEFRAG we need to make sure that bit is cleared so we dont leak it. Thanks, Signed-off-by: Josef Bacik --- fs/btrfs/inode.c | 46 ++ 1 files changed, 26 insertions(+), 20 deletions(-) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index a60be02..686ba5c 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -490,8 +490,9 @@ cont: * Unlock and free up our temp pages. */ extent_clear_unlock_delalloc(inode, start, end, NULL, -EXTENT_DIRTY | -EXTENT_DELALLOC, +EXTENT_DELALLOC | +EXTENT_DO_ACCOUNTING | +EXTENT_DEFRAG, PAGE_UNLOCK | PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | @@ -593,9 +594,10 @@ free_pages_out: cleanup_and_out: extent_clear_unlock_delalloc(inode, start, end, NULL, -EXTENT_DIRTY | EXTENT_DELALLOC, -PAGE_UNLOCK | PAGE_CLEAR_DIRTY | -PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); +EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING | +EXTENT_DEFRAG, PAGE_UNLOCK | +PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | +PAGE_END_WRITEBACK); if (!trans || IS_ERR(trans)) btrfs_error(root->fs_info, ret, "Failed to join transaction"); else @@ -770,8 +772,8 @@ retry: extent_clear_unlock_delalloc(inode, async_extent->start, async_extent->start + async_extent->ram_size - 1, - NULL, EXTENT_LOCKED | EXTENT_DELALLOC | - EXTENT_DIRTY, PAGE_UNLOCK | PAGE_CLEAR_DIRTY | + NULL, EXTENT_LOCKED | EXTENT_DELALLOC, + PAGE_UNLOCK | PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK); ret = btrfs_submit_compressed_write(inode, async_extent->start, @@ -795,9 +797,9 @@ out_free: async_extent->start + async_extent->ram_size - 1, NULL, EXTENT_LOCKED | EXTENT_DELALLOC | -EXTENT_DIRTY, PAGE_UNLOCK | -PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | -PAGE_END_WRITEBACK); +EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING, +PAGE_UNLOCK | PAGE_CLEAR_DIRTY | +PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); kfree(async_extent); goto again; } @@ -884,7 +886,7 @@ static noinline int __cow_file_range(struct btrfs_trans_handle *trans, if (ret == 0) { extent_clear_unlock_delalloc(inode, start, end, NULL, EXTENT_LOCKED | EXTENT_DELALLOC | -EXTENT_DIRTY, PAGE_UNLOCK | +EXTENT_DEFRAG, PAGE_UNLOCK | PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); @@ -995,10 +997,10 @@ out_reserve: btrfs_free_reserved_extent(root, ins.objectid, ins.offset); out_unlock: extent_clear_unlock_delalloc(inode, start, end, locked_page, -EXTENT_LOCKED | EXTENT_DIRTY | -EXTENT_DELALLOC, PAGE_UNLOCK | -PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | -PAGE_END_WRITEBACK); +EXTENT_LOCKED | EXTENT_DO_ACCOUNTING | +EXTENT_DELALLOC | EXTENT_DEFRAG, +PAGE_UNLOCK | PAGE_CLEAR_DIRTY | +PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); goto out; } @@ -1016,7 +1018,8 @@ static
Re: [PATCH] xfstests: generic/315: add one more sync and more output
On 7/29/13 12:21 PM, Josef Bacik wrote: > So df in btrfs is tricky at best, and relying on it for accurate information > is > not great, but it's the best way to verify this test. So we need to add > another > sync to make sure the pinned blocks are all freed up and the df space is > really > really accurate, otherwise we end up with this test failling because the df > after the test is slightly off (in my case it was like 36kb off). With this > patch I'm not seeing random failures of this test. Thanks, Honest question: does one more sync make this deterministic, or is it a best-effort, um, hack? -Eric > Signed-off-by: Josef Bacik > --- > tests/generic/315 |3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/tests/generic/315 b/tests/generic/315 > index 7cfc40d..7c55b8a 100644 > --- a/tests/generic/315 > +++ b/tests/generic/315 > @@ -70,10 +70,11 @@ fsize=`ls -l $TEST_DIR/testfile.$seq | awk '{print $5}'` > # Truncate the file size back to 0 > truncate -s 0 $TEST_DIR/testfile.$seq > sync > +sync > > # Preallocated disk space should be released > avail_done=`df -P $TEST_DIR | awk 'END {print $4}'` > -[ "$avail_done" -eq "$avail_begin" ] || _fail "Available disk space > ($avail_done KiB)" > +[ "$avail_done" -eq "$avail_begin" ] || _fail "Available disk space > ($avail_done KiB) wanted ($avail_begin KiB)" > > # success, all done > exit > -- 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] xfstests: generic/315: add one more sync and more output
On 7/29/13 12:31 PM, Eric Sandeen wrote: > Honest question: does one more sync make this deterministic, or is it a > best-effort, um, hack? I'm not quite sure why even 1 sync is needed. :( I'm not sure what bug this is trying to test; if you need 2 syncs for global space stats to accurately reflect the fact that you chopped off the end of a block, maybe that's ... still a bug? Or if it's just the big-hammer question of "does the truncated space *ever* get freed?" then maybe umount/remount/check would tell you that more definitively. -Eric -- 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: optimize function btrfs_read_chunk_tree
After reading all device items from the chunk tree, don't exit the loop and then navigate down the tree again to find the chunk items. Instead just read all device items and chunk items with a single tree search. This is possible because all device items are found before any chunk item in the chunks tree. Signed-off-by: Filipe David Borba Manana --- V2: Simplified logic inside the loop (suggested by Josef Bacik on irc). fs/btrfs/volumes.c | 30 ++ 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 090f57c..45c5ec3 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -5676,14 +5676,13 @@ int btrfs_read_chunk_tree(struct btrfs_root *root) mutex_lock(&uuid_mutex); lock_chunks(root); - /* first we search for all of the device items, and then we -* read in all of the chunk items. This way we can create chunk -* mappings that reference all of the devices that are afound -*/ + /* Read all device items, and then all the chunk items. All + device items are found before any chunk item (their object id + is smaller than the lowest possible object id for a chunk + item - BTRFS_FIRST_CHUNK_TREE_OBJECTID). */ key.objectid = BTRFS_DEV_ITEMS_OBJECTID; key.offset = 0; key.type = 0; -again: ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); if (ret < 0) goto error; @@ -5699,17 +5698,13 @@ again: break; } btrfs_item_key_to_cpu(leaf, &found_key, slot); - if (key.objectid == BTRFS_DEV_ITEMS_OBJECTID) { - if (found_key.objectid != BTRFS_DEV_ITEMS_OBJECTID) - break; - if (found_key.type == BTRFS_DEV_ITEM_KEY) { - struct btrfs_dev_item *dev_item; - dev_item = btrfs_item_ptr(leaf, slot, + if (found_key.type == BTRFS_DEV_ITEM_KEY) { + struct btrfs_dev_item *dev_item; + dev_item = btrfs_item_ptr(leaf, slot, struct btrfs_dev_item); - ret = read_one_dev(root, leaf, dev_item); - if (ret) - goto error; - } + ret = read_one_dev(root, leaf, dev_item); + if (ret) + goto error; } else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) { struct btrfs_chunk *chunk; chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk); @@ -5719,11 +5714,6 @@ again: } path->slots[0]++; } - if (key.objectid == BTRFS_DEV_ITEMS_OBJECTID) { - key.objectid = 0; - btrfs_release_path(path); - goto again; - } ret = 0; error: unlock_chunks(root); -- 1.7.9.5 -- 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] xfstests: generic/315: add one more sync and more output
On Mon, Jul 29, 2013 at 12:38:25PM -0500, Eric Sandeen wrote: > On 7/29/13 12:31 PM, Eric Sandeen wrote: > > Honest question: does one more sync make this deterministic, or is it a > > best-effort, um, hack? > > I'm not quite sure why even 1 sync is needed. :( > Because of COW, we won't free up the data space until the transaction commits because it is pinned, so doing the truncate and then immediately doing df will show no difference. > I'm not sure what bug this is trying to test; if you need 2 syncs for global > space stats to accurately reflect the fact that you chopped off the end of a > block, maybe that's ... still a bug? > No, it's just COW for you, in this case we do our sync, stuff gets updated and some metadata is cow'ed for once reason or another and now df doesn't quite match up (in my case it was off by like 9 blocks), doing a second sync clears these out and then df's match. > Or if it's just the big-hammer question of "does the truncated space *ever* > get freed?" then maybe umount/remount/check would tell you that more > definitively. Yeah but I think I'll do what you suggested on IRC and just use _within_range. Thanks, Josef -- 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: optimize function btrfs_read_chunk_tree
After reading all device items from the chunk tree, don't exit the loop and then navigate down the tree again to find the chunk items. Instead just read all device items and chunk items with a single tree search. This is possible because all device items are found before any chunk item in the chunks tree. This is a port of the corresponding kernel patch to keep both kernel and btrfs-progs identical: https://patchwork.kernel.org/patch/2835105/ Signed-off-by: Filipe David Borba Manana --- V2: Simplified logic inside the loop (suggested by Josef Bacik on irc). volumes.c | 28 +--- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/volumes.c b/volumes.c index 0ff2283..2c69f28 100644 --- a/volumes.c +++ b/volumes.c @@ -1718,14 +1718,13 @@ int btrfs_read_chunk_tree(struct btrfs_root *root) if (!path) return -ENOMEM; - /* first we search for all of the device items, and then we -* read in all of the chunk items. This way we can create chunk -* mappings that reference all of the devices that are afound -*/ + /* Read all device items, and then all the chunk items. All + device items are found before any chunk item (their object id + is smaller than the lowest possible object id for a chunk + item - BTRFS_FIRST_CHUNK_TREE_OBJECTID). */ key.objectid = BTRFS_DEV_ITEMS_OBJECTID; key.offset = 0; key.type = 0; -again: ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); while(1) { leaf = path->nodes[0]; @@ -1739,16 +1738,12 @@ again: break; } btrfs_item_key_to_cpu(leaf, &found_key, slot); - if (key.objectid == BTRFS_DEV_ITEMS_OBJECTID) { - if (found_key.objectid != BTRFS_DEV_ITEMS_OBJECTID) - break; - if (found_key.type == BTRFS_DEV_ITEM_KEY) { - struct btrfs_dev_item *dev_item; - dev_item = btrfs_item_ptr(leaf, slot, + if (found_key.type == BTRFS_DEV_ITEM_KEY) { + struct btrfs_dev_item *dev_item; + dev_item = btrfs_item_ptr(leaf, slot, struct btrfs_dev_item); - ret = read_one_dev(root, leaf, dev_item); - BUG_ON(ret); - } + ret = read_one_dev(root, leaf, dev_item); + BUG_ON(ret); } else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) { struct btrfs_chunk *chunk; chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk); @@ -1757,11 +1752,6 @@ again: } path->slots[0]++; } - if (key.objectid == BTRFS_DEV_ITEMS_OBJECTID) { - key.objectid = 0; - btrfs_release_path(root, path); - goto again; - } ret = 0; error: -- 1.7.9.5 -- 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] Btrfs-progs: return immediately on tree search failure
If the chunk tree search failed in volumes.c:btrfs_read_chunk_tree() return immediately, rather than looping and use the invalid contents of the path structure, causing weird errors/crash at run time. Signed-off-by: Filipe David Borba Manana --- volumes.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/volumes.c b/volumes.c index 0ff2283..79008de 100644 --- a/volumes.c +++ b/volumes.c @@ -1727,6 +1727,8 @@ int btrfs_read_chunk_tree(struct btrfs_root *root) key.type = 0; again: ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); + if (ret < 0) + goto error; while(1) { leaf = path->nodes[0]; slot = path->slots[0]; -- 1.7.9.5 -- 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] xfstests: generic/315: allow a little tolerance for our used check
So df in btrfs is tricky at best, and relying on it for accurate information is not great, but it's the best way to verify this test. To get around btrfs being inconsistent sometimes just use _within_tolerance to check our new df value to make sure that our truncate did something. With this patch I no longer see transient failures of this test. Thanks, Signed-off-by: Josef Bacik --- tests/generic/315 |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tests/generic/315 b/tests/generic/315 index 7cfc40d..9c01b5e 100644 --- a/tests/generic/315 +++ b/tests/generic/315 @@ -73,7 +73,8 @@ sync # Preallocated disk space should be released avail_done=`df -P $TEST_DIR | awk 'END {print $4}'` -[ "$avail_done" -eq "$avail_begin" ] || _fail "Available disk space ($avail_done KiB)" +_within_tolerance "df" $avail_done $avail_begin 1% +[ $? -eq 0 ] || _fail "Available disk space ($avail_done KiB) wanted ($avail_begin KiB)" # success, all done exit -- 1.7.7.6 -- 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] Btrfs: fix what bits we clear when erroring out from delalloc V2
First of all we no longer set EXTENT_DIRTY when we dirty an extent so this patch removes the clearing of EXTENT_DIRTY since it confuses me. This patch also adds clearing EXTENT_DEFRAG and also doing EXTENT_DO_ACCOUNTING when we have errors. This is because if we are clearing delalloc without adding an ordered extent then we need to make sure the enospc handling stuff is accounted for. Also if this range was DEFRAG we need to make sure that bit is cleared so we dont leak it. Thanks, Signed-off-by: Josef Bacik --- V1->V2: - only do EXTENT_DO_ACCOUNTING in compress_file_range if we had an error fs/btrfs/inode.c | 49 - 1 files changed, 28 insertions(+), 21 deletions(-) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index a60be02..88fe045 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -484,15 +484,17 @@ cont: compress_type, pages); } if (ret <= 0) { + unsigned long clear_flags = EXTENT_DELALLOC | + EXTENT_DEFRAG; + clear_flags |= (ret < 0) ? EXTENT_DO_ACCOUNTING : 0; + /* * inline extent creation worked or returned error, * we don't need to create any more async work items. * Unlock and free up our temp pages. */ extent_clear_unlock_delalloc(inode, start, end, NULL, -EXTENT_DIRTY | -EXTENT_DELALLOC, -PAGE_UNLOCK | +clear_flags, PAGE_UNLOCK | PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); @@ -593,9 +595,10 @@ free_pages_out: cleanup_and_out: extent_clear_unlock_delalloc(inode, start, end, NULL, -EXTENT_DIRTY | EXTENT_DELALLOC, -PAGE_UNLOCK | PAGE_CLEAR_DIRTY | -PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); +EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING | +EXTENT_DEFRAG, PAGE_UNLOCK | +PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | +PAGE_END_WRITEBACK); if (!trans || IS_ERR(trans)) btrfs_error(root->fs_info, ret, "Failed to join transaction"); else @@ -770,8 +773,8 @@ retry: extent_clear_unlock_delalloc(inode, async_extent->start, async_extent->start + async_extent->ram_size - 1, - NULL, EXTENT_LOCKED | EXTENT_DELALLOC | - EXTENT_DIRTY, PAGE_UNLOCK | PAGE_CLEAR_DIRTY | + NULL, EXTENT_LOCKED | EXTENT_DELALLOC, + PAGE_UNLOCK | PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK); ret = btrfs_submit_compressed_write(inode, async_extent->start, @@ -795,9 +798,9 @@ out_free: async_extent->start + async_extent->ram_size - 1, NULL, EXTENT_LOCKED | EXTENT_DELALLOC | -EXTENT_DIRTY, PAGE_UNLOCK | -PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | -PAGE_END_WRITEBACK); +EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING, +PAGE_UNLOCK | PAGE_CLEAR_DIRTY | +PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); kfree(async_extent); goto again; } @@ -884,7 +887,7 @@ static noinline int __cow_file_range(struct btrfs_trans_handle *trans, if (ret == 0) { extent_clear_unlock_delalloc(inode, start, end, NULL, EXTENT_LOCKED | EXTENT_DELALLOC | -EXTENT_DIRTY, PAGE_UNLOCK | +EXTENT_DEFRAG, PAGE_UNLOCK | PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); @@ -995,10 +998,10 @@ out_reserve: btrfs_free_reserved_extent(root, ins.objectid, ins.offset); out_unlock: extent_clear_unlock_delalloc(inode, start, end, locked_page, -EXTENT_LOCKED | EXTENT_DIRTY | -
Re: Recovering from btrfs error Couldn't read chunk root.
Unrecoverable? I know i cant mount and have access but my data are still there intact ( as i was using them till the reboot) i shouldn't be able to extract/recover them to another disc? With any magic command without mounting? Any other solutions? http://bpaste.net/show/118112/ On 29 July 2013 15:06, Wang Shilong wrote: > > 在 2013-7-29,上午2:12,Kyriakos 写道: > >> Just tried it as you said with the -v option enabled >> This is my output: >> >> http://bpaste.net/show/118112/ >> >> > This is a *long* email, and seems that btrfs list refuse it. > >> Device extent: devid = 1, start = 1667558801408, len = 1073741824, >> chunk offset = 1663255445504 >> Couldn't map the block 626309926912 >> btrfs: volumes.c:1020: btrfs_num_copies: Assertion `!(ce->start > >> logical || ce->start + ce->size < logical)' failed. >> Aborted (core dumped) > > Strange enough, we don't find any chunks during scanning process. > > And seems this is unrecoverable ~_~ > > > Wang, >> >> >> Any thoughts? >> >> On 28 July 2013 08:17, Wang Shilong wrote: >>> Hello, >>> >>> It seems Btrfs Chunk Tree is damaged, so you can not mount Btrfs filesystem >>> any more. >>> >>> However, you can try the latest Btrfs-progs, Miao Xie implements chunk tree >>> recover function. >>> >>> The url is: >>> >>>git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs.git >>> >>> >>> you can try it: >>>btrfs chunk-recover -v >>> >>> This is Time-consuming, because it will scan the whole disk. And also, >>> please catch output of processing(this is helpful to us if the recovery >>> fails, -v option >>> enable this). >>> >>> Thanks, >>> Wang >>> >>> >>> >>> >>> >>> > -- 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: add missing path alloc return value check
Also remove unused path in extent-tree.c:finish_current_insert(). Signed-off-by: Filipe David Borba Manana --- V2: added 1 more path alloc check and removed unnecessary path allocation in extent-tree.c:finish_current_insert(). extent-tree.c |8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/extent-tree.c b/extent-tree.c index f597e16..e4adaa3 100644 --- a/extent-tree.c +++ b/extent-tree.c @@ -1487,6 +1487,8 @@ int btrfs_lookup_extent_info(struct btrfs_trans_handle *trans, } path = btrfs_alloc_path(); + if (!path) + return -ENOMEM; path->reada = 1; key.objectid = bytenr; @@ -1577,6 +1579,8 @@ int btrfs_set_block_flags(struct btrfs_trans_handle *trans, BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA); path = btrfs_alloc_path(); + if (!path) + return -ENOMEM; path->reada = 1; key.objectid = bytenr; @@ -2078,7 +2082,6 @@ static int finish_current_insert(struct btrfs_trans_handle *trans, u64 end; u64 priv; struct btrfs_fs_info *info = extent_root->fs_info; - struct btrfs_path *path; struct pending_extent_op *extent_op; struct btrfs_key key; int ret; @@ -2086,8 +2089,6 @@ static int finish_current_insert(struct btrfs_trans_handle *trans, btrfs_fs_incompat(extent_root->fs_info, BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA); - path = btrfs_alloc_path(); - while(1) { ret = find_first_extent_bit(&info->extent_ins, 0, &start, &end, EXTENT_LOCKED); @@ -2121,7 +2122,6 @@ static int finish_current_insert(struct btrfs_trans_handle *trans, GFP_NOFS); kfree(extent_op); } - btrfs_free_path(path); return 0; } -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] Btrfs-progs: add missing path alloc return value check
Also remove unused path in extent-tree.c:finish_current_insert(). Signed-off-by: Filipe David Borba Manana --- V2: added 1 more path alloc check and removed unnecessary path allocation in extent-tree.c:finish_current_insert(). V3: added missing path alloc checks to dir-item.c, file-item.c and btrfs-corrupt-block.c too. btrfs-corrupt-block.c |2 ++ dir-item.c|2 ++ extent-tree.c |8 file-item.c |2 ++ 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c index 8176fad..22facd4 100644 --- a/btrfs-corrupt-block.c +++ b/btrfs-corrupt-block.c @@ -159,6 +159,8 @@ static int corrupt_extent(struct btrfs_trans_handle *trans, int should_del = rand() % 3; path = btrfs_alloc_path(); + if (!path) + return -ENOMEM; key.objectid = bytenr; key.type = (u8)-1; diff --git a/dir-item.c b/dir-item.c index f00485a..0ab3c5e 100644 --- a/dir-item.c +++ b/dir-item.c @@ -123,6 +123,8 @@ int btrfs_insert_dir_item(struct btrfs_trans_handle *trans, struct btrfs_root btrfs_set_key_type(&key, BTRFS_DIR_ITEM_KEY); key.offset = btrfs_name_hash(name, name_len); path = btrfs_alloc_path(); + if (!path) + return -ENOMEM; data_size = sizeof(*dir_item) + name_len; dir_item = insert_with_overflow(trans, root, path, &key, data_size, name, name_len); diff --git a/extent-tree.c b/extent-tree.c index f597e16..e4adaa3 100644 --- a/extent-tree.c +++ b/extent-tree.c @@ -1487,6 +1487,8 @@ int btrfs_lookup_extent_info(struct btrfs_trans_handle *trans, } path = btrfs_alloc_path(); + if (!path) + return -ENOMEM; path->reada = 1; key.objectid = bytenr; @@ -1577,6 +1579,8 @@ int btrfs_set_block_flags(struct btrfs_trans_handle *trans, BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA); path = btrfs_alloc_path(); + if (!path) + return -ENOMEM; path->reada = 1; key.objectid = bytenr; @@ -2078,7 +2082,6 @@ static int finish_current_insert(struct btrfs_trans_handle *trans, u64 end; u64 priv; struct btrfs_fs_info *info = extent_root->fs_info; - struct btrfs_path *path; struct pending_extent_op *extent_op; struct btrfs_key key; int ret; @@ -2086,8 +2089,6 @@ static int finish_current_insert(struct btrfs_trans_handle *trans, btrfs_fs_incompat(extent_root->fs_info, BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA); - path = btrfs_alloc_path(); - while(1) { ret = find_first_extent_bit(&info->extent_ins, 0, &start, &end, EXTENT_LOCKED); @@ -2121,7 +2122,6 @@ static int finish_current_insert(struct btrfs_trans_handle *trans, GFP_NOFS); kfree(extent_op); } - btrfs_free_path(path); return 0; } diff --git a/file-item.c b/file-item.c index 9c787f0..82bf99e 100644 --- a/file-item.c +++ b/file-item.c @@ -417,6 +417,8 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans, root = root->fs_info->csum_root; path = btrfs_alloc_path(); + if (!path) + return -ENOMEM; while (1) { key.objectid = BTRFS_EXTENT_CSUM_OBJECTID; -- 1.7.9.5 -- 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: Online data deduplication
On Mon, Jul 29, 2013 at 09:05:42PM +0530, Hemanth Kumar wrote: > Hello, > I am willing to perform QA on online data deduplication. From where > can i download the patches? Hi Hemanth Kumar H C, I really appreciate this :) Right now I'm planning v5 version patch set, which will come out probably in this week(Only one bug is left on my hands) v5 version will have a big change on disk format, performance, btw. For v4 you can find it in the following url: https://patchwork.kernel.org/patch/2565601/ https://patchwork.kernel.org/patch/2565591/ thanks, -liubo -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs: optimize function btrfs_read_chunk_tree
On mon, 29 Jul 2013 19:22:34 +0100, Filipe David Borba Manana wrote: > After reading all device items from the chunk tree, don't > exit the loop and then navigate down the tree again to find > the chunk items. Instead just read all device items and > chunk items with a single tree search. This is possible > because all device items are found before any chunk item in > the chunks tree. > > Signed-off-by: Filipe David Borba Manana > --- > > V2: Simplified logic inside the loop > (suggested by Josef Bacik on irc). > > fs/btrfs/volumes.c | 30 ++ > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 090f57c..45c5ec3 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -5676,14 +5676,13 @@ int btrfs_read_chunk_tree(struct btrfs_root *root) > mutex_lock(&uuid_mutex); > lock_chunks(root); > > - /* first we search for all of the device items, and then we > - * read in all of the chunk items. This way we can create chunk > - * mappings that reference all of the devices that are afound > - */ > + /* Read all device items, and then all the chunk items. All > +device items are found before any chunk item (their object id > +is smaller than the lowest possible object id for a chunk > +item - BTRFS_FIRST_CHUNK_TREE_OBJECTID). */ According to the coding style of the kernel(Documentation/CodingStyle), the preferred style for long (multi-line) comments is: /* * This is the preferred style for multi-line * comments in the Linux kernel source code. * * Description: A column of asterisks on the left side, * with beginning and ending almost-blank lines. */ The other code is OK. Reviewed-by: Miao Xie > key.objectid = BTRFS_DEV_ITEMS_OBJECTID; > key.offset = 0; > key.type = 0; > -again: > ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); > if (ret < 0) > goto error; > @@ -5699,17 +5698,13 @@ again: > break; > } > btrfs_item_key_to_cpu(leaf, &found_key, slot); > - if (key.objectid == BTRFS_DEV_ITEMS_OBJECTID) { > - if (found_key.objectid != BTRFS_DEV_ITEMS_OBJECTID) > - break; > - if (found_key.type == BTRFS_DEV_ITEM_KEY) { > - struct btrfs_dev_item *dev_item; > - dev_item = btrfs_item_ptr(leaf, slot, > + if (found_key.type == BTRFS_DEV_ITEM_KEY) { > + struct btrfs_dev_item *dev_item; > + dev_item = btrfs_item_ptr(leaf, slot, > struct btrfs_dev_item); > - ret = read_one_dev(root, leaf, dev_item); > - if (ret) > - goto error; > - } > + ret = read_one_dev(root, leaf, dev_item); > + if (ret) > + goto error; > } else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) { > struct btrfs_chunk *chunk; > chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk); > @@ -5719,11 +5714,6 @@ again: > } > path->slots[0]++; > } > - if (key.objectid == BTRFS_DEV_ITEMS_OBJECTID) { > - key.objectid = 0; > - btrfs_release_path(path); > - goto again; > - } > ret = 0; > error: > unlock_chunks(root); > -- 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 v3] Btrfs-progs: add missing path alloc return value check
On mon, 29 Jul 2013 23:37:19 +0100, Filipe David Borba Manana wrote: > Also remove unused path in extent-tree.c:finish_current_insert(). > > Signed-off-by: Filipe David Borba Manana > --- > > V2: added 1 more path alloc check and removed unnecessary path > allocation in extent-tree.c:finish_current_insert(). > V3: added missing path alloc checks to dir-item.c, file-item.c > and btrfs-corrupt-block.c too. fixup_extent_refs() in cmds-check.c and btrfs_add_root_ref() in root-tree.c also miss the path allocation check. Thanks Miao > > btrfs-corrupt-block.c |2 ++ > dir-item.c|2 ++ > extent-tree.c |8 > file-item.c |2 ++ > 4 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c > index 8176fad..22facd4 100644 > --- a/btrfs-corrupt-block.c > +++ b/btrfs-corrupt-block.c > @@ -159,6 +159,8 @@ static int corrupt_extent(struct btrfs_trans_handle > *trans, > int should_del = rand() % 3; > > path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > > key.objectid = bytenr; > key.type = (u8)-1; > diff --git a/dir-item.c b/dir-item.c > index f00485a..0ab3c5e 100644 > --- a/dir-item.c > +++ b/dir-item.c > @@ -123,6 +123,8 @@ int btrfs_insert_dir_item(struct btrfs_trans_handle > *trans, struct btrfs_root > btrfs_set_key_type(&key, BTRFS_DIR_ITEM_KEY); > key.offset = btrfs_name_hash(name, name_len); > path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > data_size = sizeof(*dir_item) + name_len; > dir_item = insert_with_overflow(trans, root, path, &key, data_size, > name, name_len); > diff --git a/extent-tree.c b/extent-tree.c > index f597e16..e4adaa3 100644 > --- a/extent-tree.c > +++ b/extent-tree.c > @@ -1487,6 +1487,8 @@ int btrfs_lookup_extent_info(struct btrfs_trans_handle > *trans, > } > > path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > path->reada = 1; > > key.objectid = bytenr; > @@ -1577,6 +1579,8 @@ int btrfs_set_block_flags(struct btrfs_trans_handle > *trans, > BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA); > > path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > path->reada = 1; > > key.objectid = bytenr; > @@ -2078,7 +2082,6 @@ static int finish_current_insert(struct > btrfs_trans_handle *trans, > u64 end; > u64 priv; > struct btrfs_fs_info *info = extent_root->fs_info; > - struct btrfs_path *path; > struct pending_extent_op *extent_op; > struct btrfs_key key; > int ret; > @@ -2086,8 +2089,6 @@ static int finish_current_insert(struct > btrfs_trans_handle *trans, > btrfs_fs_incompat(extent_root->fs_info, > BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA); > > - path = btrfs_alloc_path(); > - > while(1) { > ret = find_first_extent_bit(&info->extent_ins, 0, &start, > &end, EXTENT_LOCKED); > @@ -2121,7 +2122,6 @@ static int finish_current_insert(struct > btrfs_trans_handle *trans, > GFP_NOFS); > kfree(extent_op); > } > - btrfs_free_path(path); > return 0; > } > > diff --git a/file-item.c b/file-item.c > index 9c787f0..82bf99e 100644 > --- a/file-item.c > +++ b/file-item.c > @@ -417,6 +417,8 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans, > root = root->fs_info->csum_root; > > path = btrfs_alloc_path(); > + if (!path) > + return -ENOMEM; > > while (1) { > key.objectid = BTRFS_EXTENT_CSUM_OBJECTID; > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Btrfs-progs: optimize function btrfs_read_chunk_tree
On mon, 29 Jul 2013 19:35:49 +0100, Filipe David Borba Manana wrote: > After reading all device items from the chunk tree, don't > exit the loop and then navigate down the tree again to find > the chunk items. Instead just read all device items and > chunk items with a single tree search. This is possible > because all device items are found before any chunk item in > the chunks tree. > > This is a port of the corresponding kernel patch to keep both > kernel and btrfs-progs identical: > > https://patchwork.kernel.org/patch/2835105/ > > Signed-off-by: Filipe David Borba Manana > --- > > V2: Simplified logic inside the loop > (suggested by Josef Bacik on irc). The comment is the same to the kernel side(Coding style problem of the comment). Reviewed-by: Miao Xie > > volumes.c | 28 +--- > 1 file changed, 9 insertions(+), 19 deletions(-) > > diff --git a/volumes.c b/volumes.c > index 0ff2283..2c69f28 100644 > --- a/volumes.c > +++ b/volumes.c > @@ -1718,14 +1718,13 @@ int btrfs_read_chunk_tree(struct btrfs_root *root) > if (!path) > return -ENOMEM; > > - /* first we search for all of the device items, and then we > - * read in all of the chunk items. This way we can create chunk > - * mappings that reference all of the devices that are afound > - */ > + /* Read all device items, and then all the chunk items. All > +device items are found before any chunk item (their object id > +is smaller than the lowest possible object id for a chunk > +item - BTRFS_FIRST_CHUNK_TREE_OBJECTID). */ > key.objectid = BTRFS_DEV_ITEMS_OBJECTID; > key.offset = 0; > key.type = 0; > -again: > ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); > while(1) { > leaf = path->nodes[0]; > @@ -1739,16 +1738,12 @@ again: > break; > } > btrfs_item_key_to_cpu(leaf, &found_key, slot); > - if (key.objectid == BTRFS_DEV_ITEMS_OBJECTID) { > - if (found_key.objectid != BTRFS_DEV_ITEMS_OBJECTID) > - break; > - if (found_key.type == BTRFS_DEV_ITEM_KEY) { > - struct btrfs_dev_item *dev_item; > - dev_item = btrfs_item_ptr(leaf, slot, > + if (found_key.type == BTRFS_DEV_ITEM_KEY) { > + struct btrfs_dev_item *dev_item; > + dev_item = btrfs_item_ptr(leaf, slot, > struct btrfs_dev_item); > - ret = read_one_dev(root, leaf, dev_item); > - BUG_ON(ret); > - } > + ret = read_one_dev(root, leaf, dev_item); > + BUG_ON(ret); > } else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) { > struct btrfs_chunk *chunk; > chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk); > @@ -1757,11 +1752,6 @@ again: > } > path->slots[0]++; > } > - if (key.objectid == BTRFS_DEV_ITEMS_OBJECTID) { > - key.objectid = 0; > - btrfs_release_path(root, path); > - goto again; > - } > > ret = 0; > error: > -- 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: use list_for_each_entry_safe() when delete items
On mon, 29 Jul 2013 11:48:32 +0400, Azat Khuzhin wrote: > On Sat, Jul 27, 2013 at 2:12 PM, Azat Khuzhin wrote: >> Replace list_for_each_entry() by list_for_each_entry_safe() in >> __btrfs_close_devices() >> >> There is another place that delete items lock_stripe_add(), but there we >> don't need safe version, because after deleting we exit from loop. >> >> Signed-off-by: Azat Khuzhin >> --- >> fs/btrfs/volumes.c |4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >> index 78b8717..1d1b595 100644 >> --- a/fs/btrfs/volumes.c >> +++ b/fs/btrfs/volumes.c >> @@ -616,13 +616,13 @@ static void free_device(struct rcu_head *head) >> >> static int __btrfs_close_devices(struct btrfs_fs_devices *fs_devices) >> { >> - struct btrfs_device *device; >> + struct btrfs_device *device, *next; >> >> if (--fs_devices->opened > 0) >> return 0; >> >> mutex_lock(&fs_devices->device_list_mutex); >> - list_for_each_entry(device, &fs_devices->devices, dev_list) { >> + list_for_each_entry_safe(device, next, &fs_devices->devices, >> dev_list) { >> struct btrfs_device *new_device; >> struct rcu_string *name; > > There is "kfree(device);" at the end of loop, maybe there must "goto > again;" after it? > (instead of this patch) Your fix is right, we needn't search from the head once again. The other fix way is: call_rcu(&device->rcu, free_device); + device = new_device; } but from the viewpoint of the readability, this way is not so good. Reviewed-by: Miao Xie > >> >> -- >> 1.7.10.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: [PATCH] xfstests: generic/315: allow a little tolerance for our used check
On 07/30/2013 02:55 AM, Josef Bacik wrote: > So df in btrfs is tricky at best, and relying on it for accurate information > is > not great, but it's the best way to verify this test. To get around btrfs > being > inconsistent sometimes just use _within_tolerance to check our new df value to > make sure that our truncate did something. With this patch I no longer see > transient failures of this test. Thanks, > > Signed-off-by: Josef Bacik > --- > tests/generic/315 |3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/tests/generic/315 b/tests/generic/315 > index 7cfc40d..9c01b5e 100644 > --- a/tests/generic/315 > +++ b/tests/generic/315 > @@ -73,7 +73,8 @@ sync > > # Preallocated disk space should be released > avail_done=`df -P $TEST_DIR | awk 'END {print $4}'` > -[ "$avail_done" -eq "$avail_begin" ] || _fail "Available disk space > ($avail_done KiB)" > +_within_tolerance "df" $avail_done $avail_begin 1% > +[ $? -eq 0 ] || _fail "Available disk space ($avail_done KiB) wanted > ($avail_begin KiB)" Looks good to me. Reviewed-by: Jie Liu Thanks, -Jeff -- 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: cleanup arguments to extent_clear_unlock_delalloc
On mon, 29 Jul 2013 11:23:00 -0400, Josef Bacik wrote: > This patch removes the io_tree argument for extent_clear_unlock_delalloc since > we always use &BTRFS_I(inode)->io_tree, and it separates out the extent tree > operations from the page operations. This way we just pass in the extent bits > we want to clear and then pass in the operations we want done to the pages. > This is because I'm going to fix what extent bits we clear in some cases and > rather than add a bunch of new flags we'll just use the actual extent bits we > want to clear. Thanks, > > Signed-off-by: Josef Bacik Reviewed-by: Miao Xie > --- > fs/btrfs/extent_io.c | 32 --- > fs/btrfs/extent_io.h | 21 +++ > fs/btrfs/inode.c | 162 > -- > 3 files changed, 85 insertions(+), 130 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index cc685dd..deaea9c 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -1678,31 +1678,21 @@ out_failed: > return found; > } > > -int extent_clear_unlock_delalloc(struct inode *inode, > - struct extent_io_tree *tree, > - u64 start, u64 end, struct page *locked_page, > - unsigned long op) > +int extent_clear_unlock_delalloc(struct inode *inode, u64 start, u64 end, > + struct page *locked_page, > + unsigned long clear_bits, > + unsigned long page_ops) > { > + struct extent_io_tree *tree = &BTRFS_I(inode)->io_tree; > int ret; > struct page *pages[16]; > unsigned long index = start >> PAGE_CACHE_SHIFT; > unsigned long end_index = end >> PAGE_CACHE_SHIFT; > unsigned long nr_pages = end_index - index + 1; > int i; > - unsigned long clear_bits = 0; > - > - if (op & EXTENT_CLEAR_UNLOCK) > - clear_bits |= EXTENT_LOCKED; > - if (op & EXTENT_CLEAR_DIRTY) > - clear_bits |= EXTENT_DIRTY; > - > - if (op & EXTENT_CLEAR_DELALLOC) > - clear_bits |= EXTENT_DELALLOC; > > clear_extent_bit(tree, start, end, clear_bits, 1, 0, NULL, GFP_NOFS); > - if (!(op & (EXTENT_CLEAR_UNLOCK_PAGE | EXTENT_CLEAR_DIRTY | > - EXTENT_SET_WRITEBACK | EXTENT_END_WRITEBACK | > - EXTENT_SET_PRIVATE2))) > + if (page_ops == 0) > return 0; > > while (nr_pages > 0) { > @@ -1711,20 +1701,20 @@ int extent_clear_unlock_delalloc(struct inode *inode, >nr_pages, ARRAY_SIZE(pages)), pages); > for (i = 0; i < ret; i++) { > > - if (op & EXTENT_SET_PRIVATE2) > + if (page_ops & PAGE_SET_PRIVATE2) > SetPagePrivate2(pages[i]); > > if (pages[i] == locked_page) { > page_cache_release(pages[i]); > continue; > } > - if (op & EXTENT_CLEAR_DIRTY) > + if (page_ops & PAGE_CLEAR_DIRTY) > clear_page_dirty_for_io(pages[i]); > - if (op & EXTENT_SET_WRITEBACK) > + if (page_ops & PAGE_SET_WRITEBACK) > set_page_writeback(pages[i]); > - if (op & EXTENT_END_WRITEBACK) > + if (page_ops & PAGE_END_WRITEBACK) > end_page_writeback(pages[i]); > - if (op & EXTENT_CLEAR_UNLOCK_PAGE) > + if (page_ops & PAGE_UNLOCK) > unlock_page(pages[i]); > page_cache_release(pages[i]); > } > diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h > index f7544af..c450620 100644 > --- a/fs/btrfs/extent_io.h > +++ b/fs/btrfs/extent_io.h > @@ -44,14 +44,11 @@ > #define EXTENT_BUFFER_DUMMY 9 > > /* these are flags for extent_clear_unlock_delalloc */ > -#define EXTENT_CLEAR_UNLOCK_PAGE 0x1 > -#define EXTENT_CLEAR_UNLOCK 0x2 > -#define EXTENT_CLEAR_DELALLOC 0x4 > -#define EXTENT_CLEAR_DIRTY0x8 > -#define EXTENT_SET_WRITEBACK 0x10 > -#define EXTENT_END_WRITEBACK 0x20 > -#define EXTENT_SET_PRIVATE2 0x40 > -#define EXTENT_CLEAR_ACCOUNTING 0x80 > +#define PAGE_UNLOCK (1 << 0) > +#define PAGE_CLEAR_DIRTY (1 << 1) > +#define PAGE_SET_WRITEBACK (1 << 2) > +#define PAGE_END_WRITEBACK (1 << 3) > +#define PAGE_SET_PRIVATE2(1 << 4) > > /* > * page->private values. Every page that is controlled by the extent > @@ -328,10 +325,10 @@ int map_private_extent_buffer(struct extent_buffer *eb, > unsigned long offset, > unsigned long *map_len); > int extent_range_clear_dirty_for_io(struct inode *inode, u64 start, u64 end); > int extent_range_redirty_for_io(struct inode *inode,
Re: [PATCH] Btrfs: fix what bits we clear when erroring out from delalloc
On mon, 29 Jul 2013 13:24:22 -0400, Josef Bacik wrote: > First of all we no longer set EXTENT_DIRTY when we dirty an extent so this > patch > removes the clearing of EXTENT_DIRTY since it confuses me. This patch also > adds > clearing EXTENT_DEFRAG and also doing EXTENT_DO_ACCOUNTING when we have > errors. > This is because if we are clearing delalloc without adding an ordered extent > then we need to make sure the enospc handling stuff is accounted for. Also if > this range was DEFRAG we need to make sure that bit is cleared so we dont leak > it. Thanks, > > Signed-off-by: Josef Bacik > --- > fs/btrfs/inode.c | 46 ++ > 1 files changed, 26 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index a60be02..686ba5c 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -490,8 +490,9 @@ cont: >* Unlock and free up our temp pages. >*/ > extent_clear_unlock_delalloc(inode, start, end, NULL, > - EXTENT_DIRTY | > - EXTENT_DELALLOC, > + EXTENT_DELALLOC | > + EXTENT_DO_ACCOUNTING | > + EXTENT_DEFRAG, >PAGE_UNLOCK | >PAGE_CLEAR_DIRTY | >PAGE_SET_WRITEBACK | I found we released the reserved space in cow_file_range_inline(), but at that time, we didn't drop the outstanding_extents counter by the number of the delalloc extents, so it might leave some reserved space which was not released. So I think we should remove the release function in cow_file_range_inline(). (This bug is not introduced by your patch. but if we don't fix the above problem before applying your patch, the double release would happen because we have released the space in cow_file_range_inline()) > @@ -593,9 +594,10 @@ free_pages_out: > > cleanup_and_out: > extent_clear_unlock_delalloc(inode, start, end, NULL, > - EXTENT_DIRTY | EXTENT_DELALLOC, > - PAGE_UNLOCK | PAGE_CLEAR_DIRTY | > - PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); > + EXTENT_DELALLOC | EXTENT_DO_ACCOUNTING | > + EXTENT_DEFRAG, PAGE_UNLOCK | > + PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | > + PAGE_END_WRITEBACK); > if (!trans || IS_ERR(trans)) > btrfs_error(root->fs_info, ret, "Failed to join transaction"); > else > @@ -770,8 +772,8 @@ retry: > extent_clear_unlock_delalloc(inode, async_extent->start, > async_extent->start + > async_extent->ram_size - 1, > - NULL, EXTENT_LOCKED | EXTENT_DELALLOC | > - EXTENT_DIRTY, PAGE_UNLOCK | PAGE_CLEAR_DIRTY | > + NULL, EXTENT_LOCKED | EXTENT_DELALLOC, > + PAGE_UNLOCK | PAGE_CLEAR_DIRTY | > PAGE_SET_WRITEBACK); > ret = btrfs_submit_compressed_write(inode, > async_extent->start, > @@ -795,9 +797,9 @@ out_free: >async_extent->start + >async_extent->ram_size - 1, >NULL, EXTENT_LOCKED | EXTENT_DELALLOC | > - EXTENT_DIRTY, PAGE_UNLOCK | > - PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | > - PAGE_END_WRITEBACK); > + EXTENT_DEFRAG | EXTENT_DO_ACCOUNTING, > + PAGE_UNLOCK | PAGE_CLEAR_DIRTY | > + PAGE_SET_WRITEBACK | PAGE_END_WRITEBACK); > kfree(async_extent); > goto again; > } > @@ -884,7 +886,7 @@ static noinline int __cow_file_range(struct > btrfs_trans_handle *trans, > if (ret == 0) { > extent_clear_unlock_delalloc(inode, start, end, NULL, >EXTENT_LOCKED | EXTENT_DELALLOC | > - EXTENT_DIRTY, PAGE_UNLOCK | > + EXTENT_DEFRAG, PAGE_UNLOCK | >PAGE_CLEAR_DIRTY | PAGE_SET_WRITEBACK | >PAGE_END_WRITEBACK); If we remove the reserved space release function in cow_file_range_inline() as I said above, we should add EXTENT_DO_ACCOUNTING here. I will send a patch to fix t