Re: [PATCH] btrfs: clear bio reference after submit_one_bio()

2015-11-07 Thread Alex Lyakas 
Hi Holger,
I think it will cause an invalid paging request, just like in case
that Naohiro has fixed.
I am not running the "latest and greatest" btrfs in my system, and it
is not easy to set it up, that's why I cannot submit patches based on
the latest code, I can only review and comment on patches.

Alex.


On Thu, Nov 5, 2015 at 3:08 PM, Holger Hoffstätte
 wrote:
> On 10/11/15 20:09, Alex Lyakas wrote:
>> Hi Naota,
>>
>> What happens if btrfs_bio_alloc() in submit_extent_page fails? Then we
>> return -ENOMEM to the caller, but we do not set *bio_ret to NULL. And
>> if *bio_ret was non-NULL upon entry into submit_extent_page, then we
>> had submitted this bio before getting to btrfs_bio_alloc(). So should
>> btrfs_bio_alloc() failure be handled in the same way?
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 3915c94..cd443bc 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -2834,8 +2834,11 @@ static int submit_extent_page(int rw, struct
>> extent_io_tree *tree,
>>
>> bio = btrfs_bio_alloc(bdev, sector, BIO_MAX_PAGES,
>> GFP_NOFS | __GFP_HIGH);
>> -   if (!bio)
>> +   if (!bio) {
>> +   if (bio_ret)
>> +   *bio_ret = NULL;
>> return -ENOMEM;
>> +   }
>>
>> bio_add_page(bio, page, page_size, offset);
>> bio->bi_end_io = end_io_func;
>>
>
> Did you get any feedback on this? It seems it could cause data loss or
> corruption on allocation failures, no?
>
> -h
>
--
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: clear bio reference after submit_one_bio()

2015-11-05 Thread Holger Hoffstätte 
On 10/11/15 20:09, Alex Lyakas wrote:
> Hi Naota,
> 
> What happens if btrfs_bio_alloc() in submit_extent_page fails? Then we
> return -ENOMEM to the caller, but we do not set *bio_ret to NULL. And
> if *bio_ret was non-NULL upon entry into submit_extent_page, then we
> had submitted this bio before getting to btrfs_bio_alloc(). So should
> btrfs_bio_alloc() failure be handled in the same way?
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 3915c94..cd443bc 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2834,8 +2834,11 @@ static int submit_extent_page(int rw, struct
> extent_io_tree *tree,
> 
> bio = btrfs_bio_alloc(bdev, sector, BIO_MAX_PAGES,
> GFP_NOFS | __GFP_HIGH);
> -   if (!bio)
> +   if (!bio) {
> +   if (bio_ret)
> +   *bio_ret = NULL;
> return -ENOMEM;
> +   }
> 
> bio_add_page(bio, page, page_size, offset);
> bio->bi_end_io = end_io_func;
> 

Did you get any feedback on this? It seems it could cause data loss or
corruption on allocation failures, no?

-h

--
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: clear bio reference after submit_one_bio()

2015-10-11 Thread Alex Lyakas 
Hi Naota,

What happens if btrfs_bio_alloc() in submit_extent_page fails? Then we
return -ENOMEM to the caller, but we do not set *bio_ret to NULL. And
if *bio_ret was non-NULL upon entry into submit_extent_page, then we
had submitted this bio before getting to btrfs_bio_alloc(). So should
btrfs_bio_alloc() failure be handled in the same way?

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 3915c94..cd443bc 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2834,8 +2834,11 @@ static int submit_extent_page(int rw, struct
extent_io_tree *tree,

bio = btrfs_bio_alloc(bdev, sector, BIO_MAX_PAGES,
GFP_NOFS | __GFP_HIGH);
-   if (!bio)
+   if (!bio) {
+   if (bio_ret)
+   *bio_ret = NULL;
return -ENOMEM;
+   }

bio_add_page(bio, page, page_size, offset);
bio->bi_end_io = end_io_func;


Thanks,
Alex.

On Wed, Jan 7, 2015 at 12:46 AM, Satoru Takeuchi
 wrote:
> Hi Naota,
>
> On 2015/01/06 1:01, Naohiro Aota wrote:
>> After submit_one_bio(), `bio' can go away. However submit_extent_page()
>> leave `bio' referable if submit_one_bio() failed (e.g. -ENOMEM on OOM).
>> It will cause invalid paging request when submit_extent_page() is called
>> next time.
>>
>> I reproduced ENOMEM case with the following script (need
>> CONFIG_FAIL_PAGE_ALLOC, and CONFIG_FAULT_INJECTION_DEBUG_FS).
>
> I confirmed that this problem reproduce with 3.19-rc3 and
> not reproduce with 3.19-rc3 with your patch.
>
> Tested-by: Satoru Takeuchi 
>
> Thank you for reporting this problem with the reproducer
> and fixing it too.
>
>   NOTE:
>   I used v3.19-rc3's tools/testing/fault-injection/failcmd.sh
>   for the following "./failcmd.sh".
>
>   >./failcmd.sh -p $percent -t $times -i $interval \
>   >--ignore-gfp-highmem=N --ignore-gfp-wait=N 
> --min-order=0 \
>   >-- \
>   >cat $directory/file > /dev/null
>
> * 3.19-rc1 + your patch
>
> ===
> # ./run
> 512+0 records in
> 512+0 records out
> #
> ===
>
> * 3.19-rc3
>
> ===
> # ./run
> 512+0 records in
> 512+0 records out
> [  188.433726] run (776): drop_caches: 1
> [  188.682372] FAULT_INJECTION: forcing a failure.
> name fail_page_alloc, interval 100, probability 111000, space 0, times 3
> [  188.689986] CPU: 0 PID: 954 Comm: cat Not tainted 3.19.0-rc3-ktest #1
> [  188.693834] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
> Bochs 01/01/2011
> [  188.698466]  0064 88007b343618 816e5563 
> 88007fc0fc78
> [  188.702730]  81c655c0 88007b343638 813851b5 
> 0010
> [  188.707043]  0002 88007b343768 81188126 
> 88007b3435a8
> [  188.711283] Call Trace:
> [  188.712620]  [] dump_stack+0x45/0x57
> [  188.715330]  [] should_fail+0x135/0x140
> [  188.718218]  [] __alloc_pages_nodemask+0xd6/0xb30
> [  188.721567]  [] ? blk_rq_map_sg+0x35/0x170
> [  188.724558]  [] ? virtio_queue_rq+0x145/0x2b0 
> [virtio_blk]
> [  188.728191]  [] ? 
> btrfs_submit_compressed_read+0xcf/0x4d0 [btrfs]
> [  188.732079]  [] ? kmem_cache_alloc+0x1cb/0x230
> [  188.735153]  [] ? mempool_alloc_slab+0x15/0x20
> [  188.738188]  [] alloc_pages_current+0x9a/0x120
> [  188.741153]  [] btrfs_submit_compressed_read+0x1a9/0x4d0 
> [btrfs]
> [  188.744835]  [] btrfs_submit_bio_hook+0x1c1/0x1d0 [btrfs]
> [  188.748225]  [] ? lookup_extent_mapping+0x13/0x20 [btrfs]
> [  188.751547]  [] ? btrfs_get_extent+0x98/0xad0 [btrfs]
> [  188.754656]  [] submit_one_bio+0x67/0xa0 [btrfs]
> [  188.757554]  [] submit_extent_page.isra.35+0xd7/0x1c0 
> [btrfs]
> [  188.760981]  [] __do_readpage+0x31d/0x7b0 [btrfs]
> [  188.763920]  [] ? btrfs_create_repair_bio+0x110/0x110 
> [btrfs]
> [  188.767382]  [] ? btrfs_submit_direct+0x7b0/0x7b0 [btrfs]
> [  188.770671]  [] ? btrfs_lookup_ordered_range+0x13d/0x180 
> [btrfs]
> [  188.774366]  [] 
> __extent_readpages.constprop.42+0x2ba/0x2d0 [btrfs]
> [  188.778031]  [] ? btrfs_submit_direct+0x7b0/0x7b0 [btrfs]
> [  188.781241]  [] extent_readpages+0x169/0x1b0 [btrfs]
> [  188.784322]  [] ? btrfs_submit_direct+0x7b0/0x7b0 [btrfs]
> [  188.789014]  [] btrfs_readpages+0x1f/0x30 [btrfs]
> [  188.792028]  [] __do_page_cache_readahead+0x18c/0x1f0
> [  188.795078]  [] ondemand_readahead+0xdf/0x260
> [  188.797702]  [] ? btrfs_congested_fn+0x5f/0xa0 [btrfs]
> [  188.800718]  [] page_cache_async_readahead+0x71/0xa0
> [  188.803650]  [] generic_file_read_iter+0x40f/0x5e0
> [  188.806480]  [] new_sync_read+0x7e/0xb0
> [  188.808832]  [] __vfs_read+0x18/0x50
> [  188.811068]  [] vfs_read+0x8a/0x140
> [  188.813298]  [] SyS_read+0x46/0xb0

Re: [PATCH] btrfs: clear bio reference after submit_one_bio()

2015-01-06 Thread Satoru Takeuchi 
Hi Naota,

On 2015/01/06 1:01, Naohiro Aota wrote:
 After submit_one_bio(), `bio' can go away. However submit_extent_page()
 leave `bio' referable if submit_one_bio() failed (e.g. -ENOMEM on OOM).
 It will cause invalid paging request when submit_extent_page() is called
 next time.
 
 I reproduced ENOMEM case with the following script (need
 CONFIG_FAIL_PAGE_ALLOC, and CONFIG_FAULT_INJECTION_DEBUG_FS).

I confirmed that this problem reproduce with 3.19-rc3 and
not reproduce with 3.19-rc3 with your patch.

Tested-by: Satoru Takeuchi takeuchi_sat...@jp.fujitsu.com

Thank you for reporting this problem with the reproducer
and fixing it too.

  NOTE:
  I used v3.19-rc3's tools/testing/fault-injection/failcmd.sh
  for the following ./failcmd.sh.

  ./failcmd.sh -p $percent -t $times -i $interval \
  --ignore-gfp-highmem=N --ignore-gfp-wait=N --min-order=0 
\
  -- \
  cat $directory/file  /dev/null

* 3.19-rc1 + your patch

===
# ./run
512+0 records in
512+0 records out
# 
===

* 3.19-rc3

===
# ./run
512+0 records in
512+0 records out
[  188.433726] run (776): drop_caches: 1
[  188.682372] FAULT_INJECTION: forcing a failure.
name fail_page_alloc, interval 100, probability 111000, space 0, times 3
[  188.689986] CPU: 0 PID: 954 Comm: cat Not tainted 3.19.0-rc3-ktest #1
[  188.693834] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
Bochs 01/01/2011
[  188.698466]  0064 88007b343618 816e5563 
88007fc0fc78
[  188.702730]  81c655c0 88007b343638 813851b5 
0010
[  188.707043]  0002 88007b343768 81188126 
88007b3435a8
[  188.711283] Call Trace:
[  188.712620]  [816e5563] dump_stack+0x45/0x57
[  188.715330]  [813851b5] should_fail+0x135/0x140
[  188.718218]  [81188126] __alloc_pages_nodemask+0xd6/0xb30
[  188.721567]  [81339075] ? blk_rq_map_sg+0x35/0x170
[  188.724558]  [a0010705] ? virtio_queue_rq+0x145/0x2b0 [virtio_blk]
[  188.728191]  [a01bd00f] ? btrfs_submit_compressed_read+0xcf/0x4d0 
[btrfs]
[  188.732079]  [811d99fb] ? kmem_cache_alloc+0x1cb/0x230
[  188.735153]  [81181265] ? mempool_alloc_slab+0x15/0x20
[  188.738188]  [811cee1a] alloc_pages_current+0x9a/0x120
[  188.741153]  [a01bd0e9] btrfs_submit_compressed_read+0x1a9/0x4d0 
[btrfs]
[  188.744835]  [a0178621] btrfs_submit_bio_hook+0x1c1/0x1d0 [btrfs]
[  188.748225]  [a018b7b3] ? lookup_extent_mapping+0x13/0x20 [btrfs]
[  188.751547]  [a0179c08] ? btrfs_get_extent+0x98/0xad0 [btrfs]
[  188.754656]  [a01901d7] submit_one_bio+0x67/0xa0 [btrfs]
[  188.757554]  [a0193f27] submit_extent_page.isra.35+0xd7/0x1c0 
[btrfs]
[  188.760981]  [a019509d] __do_readpage+0x31d/0x7b0 [btrfs]
[  188.763920]  [a0195f10] ? btrfs_create_repair_bio+0x110/0x110 
[btrfs]
[  188.767382]  [a0179b70] ? btrfs_submit_direct+0x7b0/0x7b0 [btrfs]
[  188.770671]  [a018f88d] ? btrfs_lookup_ordered_range+0x13d/0x180 
[btrfs]
[  188.774366]  [a01958ca] 
__extent_readpages.constprop.42+0x2ba/0x2d0 [btrfs]
[  188.778031]  [a0179b70] ? btrfs_submit_direct+0x7b0/0x7b0 [btrfs]
[  188.781241]  [a01969b9] extent_readpages+0x169/0x1b0 [btrfs]
[  188.784322]  [a0179b70] ? btrfs_submit_direct+0x7b0/0x7b0 [btrfs]
[  188.789014]  [a0176b0f] btrfs_readpages+0x1f/0x30 [btrfs]
[  188.792028]  [8118bf5c] __do_page_cache_readahead+0x18c/0x1f0
[  188.795078]  [8118c09f] ondemand_readahead+0xdf/0x260
[  188.797702]  [a016c5df] ? btrfs_congested_fn+0x5f/0xa0 [btrfs]
[  188.800718]  [8118c291] page_cache_async_readahead+0x71/0xa0
[  188.803650]  [8118017f] generic_file_read_iter+0x40f/0x5e0
[  188.806480]  [811f43be] new_sync_read+0x7e/0xb0
[  188.808832]  [811f55d8] __vfs_read+0x18/0x50
[  188.811068]  [811f569a] vfs_read+0x8a/0x140
[  188.813298]  [811f5796] SyS_read+0x46/0xb0
[  188.815486]  [81125806] ? __audit_syscall_exit+0x1f6/0x2a0
[  188.818293]  [816eb8e9] system_call_fastpath+0x12/0x17
[  188.821005] BUG: unable to handle kernel paging request at 0001000c
[  188.821984] IP: [a01901b3] submit_one_bio+0x43/0xa0 [btrfs]
[  188.821984] PGD 7bad3067 PUD 0 
[  188.821984] Oops:  [#1] SMP 
[  188.821984] Modules linked in: ip6table_filter ip6_tables ebtable_nat 
ebtables bnep bluetooth rfkill btrfs xor raid6_pq microcode 8139too serio_raw 
virtio_balloon 8139cp mii nfsd auth_rpcgss nfs_acl lockd grace sunrpc 
virtio_blk ata_generic pata_acpi
[  188.821984] CPU: 1 PID: 954 Comm: cat Not tainted 3.19.0-rc3-ktest #1
[