fs/fuse: fix locked kio request send

Submitted by Pavel Butsykin on May 29, 2019, 9:12 a.m.

Details

Message ID 20190529091245.2932-1-pbutsykin@virtuozzo.com
State New
Series "fs/fuse: fix locked kio request send"
Headers show

Commit Message

Pavel Butsykin May 29, 2019, 9:12 a.m.
Since patch 'fs/fuse kio: simplify processing and sending kio requests',
fuse_request_queue_background() function cannot be executed under spinlock,
because inserting kio request to RPC queue implies additional memory
allocations, such as pcs_int_request, fuse_req, pcs_ioc_getmap, pcs_map_entry,
etc.

We can release fi->lock inside fuse_send_writepage() for the duration of
execution fuse_request_queue_background(), because in this place fi->lock
protects only fi->writectr.

Call trace:
[95042.681286]  [<ffffffff87a7fbdf>] dump_stack+0x19/0x1b
[95042.681289]  [<ffffffff872f0723>] __might_sleep+0x173/0x230
[95042.681293]  [<ffffffff8748f463>] kmem_cache_alloc+0x2d3/0x430
[95042.681326]  [<ffffffffc0a40a10>] ireq_process+0x26/0x28 [fuse_kio_pcs]
[95042.681329]  [<ffffffffc0a449fb>] pcs_fuse_submit+0x4df/0x4f5 [fuse_kio_pcs]
[95042.681332]  [<ffffffffc0a456cd>] kpcs_req_send+0x1b2/0x35d [fuse_kio_pcs]
[95042.681334]  [<ffffffff872ffca5>] ? local_clock+0x25/0x30
[95042.681338]  [<ffffffff8734bdcf>] ? lock_release_holdtime.part.30+0xf/0x1a0
[95042.681342]  [<ffffffff8764387b>] ? do_raw_spin_unlock+0x4b/0x90
[95042.681345]  [<ffffffffc0962187>] flush_bg_queue+0x1c6/0x216 [fuse]
[95042.681348]  [<ffffffffc0963510>] fuse_request_queue_background+0x358/0x374 [fuse]
[95042.681353]  [<ffffffffc097562b>] fuse_send_writepage+0xd9/0x165 [fuse]
[95042.681356]  [<ffffffffc0975720>] fuse_flush_writepages+0x69/0x8f [fuse]
[95042.681359]  [<ffffffffc09763dc>] fuse_send_writepages+0x582/0x5e3 [fuse]
[95042.681362]  [<ffffffffc096f117>] ? get_fuse_conn+0x1c/0x1e [fuse]

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
---
 fs/fuse/file.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a1dc3216da87..259eef0fa005 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1950,6 +1950,8 @@  __acquires(fi->lock)
 	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
 	bool queued;
 
+	fi->writectr++;
+
 	if (inarg->offset + data_size <= size) {
 		inarg->size = data_size;
 	} else if (inarg->offset < size) {
@@ -1960,15 +1962,18 @@  __acquires(fi->lock)
 	}
 
 	req->in.args[1].size = inarg->size;
+	spin_unlock(&fi->lock);
+
 	queued = fuse_request_queue_background(fc, req);
+	spin_lock(&fi->lock);
 	/* Fails on broken connection only */
 	if (unlikely(!queued))
 		goto out_free;
 
-	fi->writectr++;
 	return;
 
  out_free:
+ 	fi->writectr--;
 	fuse_writepage_finish(fc, req);
 	spin_unlock(&fi->lock);
 	fuse_writepage_free(fc, req);

Comments

Kirill Tkhai May 29, 2019, 9:13 a.m.
On 29.05.2019 12:12, Pavel Butsykin wrote:
> Since patch 'fs/fuse kio: simplify processing and sending kio requests',
> fuse_request_queue_background() function cannot be executed under spinlock,
> because inserting kio request to RPC queue implies additional memory
> allocations, such as pcs_int_request, fuse_req, pcs_ioc_getmap, pcs_map_entry,
> etc.
> 
> We can release fi->lock inside fuse_send_writepage() for the duration of
> execution fuse_request_queue_background(), because in this place fi->lock
> protects only fi->writectr.
> 
> Call trace:
> [95042.681286]  [<ffffffff87a7fbdf>] dump_stack+0x19/0x1b
> [95042.681289]  [<ffffffff872f0723>] __might_sleep+0x173/0x230
> [95042.681293]  [<ffffffff8748f463>] kmem_cache_alloc+0x2d3/0x430
> [95042.681326]  [<ffffffffc0a40a10>] ireq_process+0x26/0x28 [fuse_kio_pcs]
> [95042.681329]  [<ffffffffc0a449fb>] pcs_fuse_submit+0x4df/0x4f5 [fuse_kio_pcs]
> [95042.681332]  [<ffffffffc0a456cd>] kpcs_req_send+0x1b2/0x35d [fuse_kio_pcs]
> [95042.681334]  [<ffffffff872ffca5>] ? local_clock+0x25/0x30
> [95042.681338]  [<ffffffff8734bdcf>] ? lock_release_holdtime.part.30+0xf/0x1a0
> [95042.681342]  [<ffffffff8764387b>] ? do_raw_spin_unlock+0x4b/0x90
> [95042.681345]  [<ffffffffc0962187>] flush_bg_queue+0x1c6/0x216 [fuse]
> [95042.681348]  [<ffffffffc0963510>] fuse_request_queue_background+0x358/0x374 [fuse]
> [95042.681353]  [<ffffffffc097562b>] fuse_send_writepage+0xd9/0x165 [fuse]
> [95042.681356]  [<ffffffffc0975720>] fuse_flush_writepages+0x69/0x8f [fuse]
> [95042.681359]  [<ffffffffc09763dc>] fuse_send_writepages+0x582/0x5e3 [fuse]
> [95042.681362]  [<ffffffffc096f117>] ? get_fuse_conn+0x1c/0x1e [fuse]
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  fs/fuse/file.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index a1dc3216da87..259eef0fa005 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1950,6 +1950,8 @@ __acquires(fi->lock)
>  	__u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
>  	bool queued;
>  
> +	fi->writectr++;
> +
>  	if (inarg->offset + data_size <= size) {
>  		inarg->size = data_size;
>  	} else if (inarg->offset < size) {
> @@ -1960,15 +1962,18 @@ __acquires(fi->lock)
>  	}
>  
>  	req->in.args[1].size = inarg->size;
> +	spin_unlock(&fi->lock);
> +
>  	queued = fuse_request_queue_background(fc, req);
> +	spin_lock(&fi->lock);
>  	/* Fails on broken connection only */
>  	if (unlikely(!queued))
>  		goto out_free;
>  
> -	fi->writectr++;
>  	return;
>  
>   out_free:
> + 	fi->writectr--;
>  	fuse_writepage_finish(fc, req);
>  	spin_unlock(&fi->lock);
>  	fuse_writepage_free(fc, req);
>
Alexey Kuznetsov May 29, 2019, 1:53 p.m.
Hello!

Ack

On Wed, May 29, 2019 at 12:13 PM Pavel Butsykin <pbutsykin@virtuozzo.com> wrote:
>
> Since patch 'fs/fuse kio: simplify processing and sending kio requests',
> fuse_request_queue_background() function cannot be executed under spinlock,
> because inserting kio request to RPC queue implies additional memory
> allocations, such as pcs_int_request, fuse_req, pcs_ioc_getmap, pcs_map_entry,
> etc.
>
> We can release fi->lock inside fuse_send_writepage() for the duration of
> execution fuse_request_queue_background(), because in this place fi->lock
> protects only fi->writectr.
>
> Call trace:
> [95042.681286]  [<ffffffff87a7fbdf>] dump_stack+0x19/0x1b
> [95042.681289]  [<ffffffff872f0723>] __might_sleep+0x173/0x230
> [95042.681293]  [<ffffffff8748f463>] kmem_cache_alloc+0x2d3/0x430
> [95042.681326]  [<ffffffffc0a40a10>] ireq_process+0x26/0x28 [fuse_kio_pcs]
> [95042.681329]  [<ffffffffc0a449fb>] pcs_fuse_submit+0x4df/0x4f5 [fuse_kio_pcs]
> [95042.681332]  [<ffffffffc0a456cd>] kpcs_req_send+0x1b2/0x35d [fuse_kio_pcs]
> [95042.681334]  [<ffffffff872ffca5>] ? local_clock+0x25/0x30
> [95042.681338]  [<ffffffff8734bdcf>] ? lock_release_holdtime.part.30+0xf/0x1a0
> [95042.681342]  [<ffffffff8764387b>] ? do_raw_spin_unlock+0x4b/0x90
> [95042.681345]  [<ffffffffc0962187>] flush_bg_queue+0x1c6/0x216 [fuse]
> [95042.681348]  [<ffffffffc0963510>] fuse_request_queue_background+0x358/0x374 [fuse]
> [95042.681353]  [<ffffffffc097562b>] fuse_send_writepage+0xd9/0x165 [fuse]
> [95042.681356]  [<ffffffffc0975720>] fuse_flush_writepages+0x69/0x8f [fuse]
> [95042.681359]  [<ffffffffc09763dc>] fuse_send_writepages+0x582/0x5e3 [fuse]
> [95042.681362]  [<ffffffffc096f117>] ? get_fuse_conn+0x1c/0x1e [fuse]
>
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  fs/fuse/file.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index a1dc3216da87..259eef0fa005 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1950,6 +1950,8 @@ __acquires(fi->lock)
>         __u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
>         bool queued;
>
> +       fi->writectr++;
> +
>         if (inarg->offset + data_size <= size) {
>                 inarg->size = data_size;
>         } else if (inarg->offset < size) {
> @@ -1960,15 +1962,18 @@ __acquires(fi->lock)
>         }
>
>         req->in.args[1].size = inarg->size;
> +       spin_unlock(&fi->lock);
> +
>         queued = fuse_request_queue_background(fc, req);
> +       spin_lock(&fi->lock);
>         /* Fails on broken connection only */
>         if (unlikely(!queued))
>                 goto out_free;
>
> -       fi->writectr++;
>         return;
>
>   out_free:
> +       fi->writectr--;
>         fuse_writepage_finish(fc, req);
>         spin_unlock(&fi->lock);
>         fuse_writepage_free(fc, req);
> --
> 2.15.1
>