[Devel] [PATCH 3/8] fuse: do not take fc->lock in fuse_request_send_background()
Pavel Butsykin
pbutsykin at virtuozzo.com
Tue Apr 23 11:51:00 MSK 2019
On 23.04.2019 11:40, Kirill Tkhai wrote:
> On 23.04.2019 11:36, Pavel Butsykin wrote:
>> On 23.04.2019 10:48, Kirill Tkhai wrote:
>>> On 23.04.2019 09:56, Pavel Butsykin wrote:
>>>>
>>>>
>>>> On 03.04.2019 18:37, Kirill Tkhai wrote:
>>>>> ms commit 63825b4e1da5
>>>>>
>>>>> Currently, we take fc->lock there only to check for fc->connected.
>>>>> But this flag is changed only on connection abort, which is very
>>>>> rare operation.
>>>>>
>>>>> So allow checking fc->connected under just fc->bg_lock and use this lock
>>>>> (as well as fc->lock) when resetting fc->connected.
>>>>>
>>>>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>>>>> Signed-off-by: Miklos Szeredi <mszeredi at redhat.com>
>>>>> ---
>>>>> fs/fuse/dev.c | 73 +++++++++++++++++++++++++++---------------------------
>>>>> fs/fuse/file.c | 4 ++-
>>>>> fs/fuse/fuse_i.h | 3 +-
>>>>> 3 files changed, 41 insertions(+), 39 deletions(-)
>>>>>
>>>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>>>>> index 1ffc10ff18ba..1355f4a0a8e4 100644
>>>>> --- a/fs/fuse/dev.c
>>>>> +++ b/fs/fuse/dev.c
>>>>> @@ -598,69 +598,70 @@ void fuse_request_send(struct fuse_conn *fc, struct fuse_req *req)
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(fuse_request_send);
>>>>>
>>>>> -/*
>>>>> - * Called under fc->lock
>>>>> - *
>>>>> - * fc->connected must have been checked previously
>>>>> - */
>>>>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>>>>> - struct fuse_req *req)
>>>>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req)
>>>>> {
>>>>> struct fuse_iqueue *fiq = req->fiq;
>>>>> + bool queued = false;
>>>>>
>>>>> - BUG_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>>>> -
>>>>> + WARN_ON(!test_bit(FR_BACKGROUND, &req->flags));
>>>>> if (!test_bit(FR_WAITING, &req->flags)) {
>>>>> __set_bit(FR_WAITING, &req->flags);
>>>>> atomic_inc(&fc->num_waiting);
>>>>> }
>>>>> __set_bit(FR_ISREPLY, &req->flags);
>>>>> spin_lock(&fc->bg_lock);
>>>>> - fc->num_background++;
>>>>> - if (fc->num_background == fc->max_background)
>>>>> - fc->blocked = 1;
>>>>> - if (fc->num_background == fc->congestion_threshold &&
>>>>> - fc->bdi_initialized) {
>>>>> - set_bdi_congested(&fc->bdi, BLK_RW_SYNC);
>>>>> - set_bdi_congested(&fc->bdi, BLK_RW_ASYNC);
>>>>> - }
>>>>> + if (likely(fc->connected)) {
>>>>> + fc->num_background++;
>>>>> + if (fc->num_background == fc->max_background)
>>>>> + fc->blocked = 1;
>>>>> + if (fc->num_background == fc->congestion_threshold && fc->sb) {
>>>>> + set_bdi_congested(fc->sb->s_bdi, BLK_RW_SYNC);
>>>>> + set_bdi_congested(fc->sb->s_bdi, BLK_RW_ASYNC);
>>>>> + }
>>>>>
>>>>> - if (test_bit(FR_NONBLOCKING, &req->flags)) {
>>>>> - fc->active_background++;
>>>>> - spin_lock(&fiq->waitq.lock);
>>>>> - req->in.h.unique = fuse_get_unique(fiq);
>>>>> - queue_request(fiq, req);
>>>>> - spin_unlock(&fiq->waitq.lock);
>>>>> - goto unlock;
>>>>> - }
>>>>> + if (test_bit(FR_NONBLOCKING, &req->flags)) {
>>>>> + fc->active_background++;
>>>>> + spin_lock(&fiq->waitq.lock);
>>>>> + req->in.h.unique = fuse_get_unique(fiq);
>>>>> + queue_request(fiq, req);
>>>>> + spin_unlock(&fiq->waitq.lock);
>>>>> + queued = true;
>>>>> + goto unlock;
>>>>> + }
>>>>>
>>>>> - list_add_tail(&req->list, &fc->bg_queue);
>>>>> - flush_bg_queue(fc, fiq);
>>>>> + list_add_tail(&req->list, &fc->bg_queue);
>>>>> + flush_bg_queue(fc, fiq);
>>>>> + queued = true;
>>>>> + }
>>>>> unlock:
>>>>> spin_unlock(&fc->bg_lock);
>>>>> + return queued;
>>>>> }
>>>>>
>>>>> void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req)
>>>>> {
>>>>> - BUG_ON(!req->end);
>>>>> + bool fail;
>>>>> + WARN_ON(!req->end);
>>>>>
>>>>> if (fc->kio.op && !fc->kio.op->req_send(fc, req, true, false))
>>>>> return;
>>>>>
>>>>> spin_lock(&fc->lock);
>>>>> - if (req->page_cache && req->ff &&
>>>>> - test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state)) {
>>>>> + fail = (req->page_cache && req->ff &&
>>>>> + test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state));
>>>>> + spin_unlock(&fc->lock);
>>>>> +
>>>>> + if (fail) {
>>>>> + /* FIXME */
>>>>
>>>> What needs to be fixed here?
>>>
>>> Commit aims to remove fc->lock from this function (and it's called "fuse: do not
>>> take fc->lock in fuse_request_send_background()). But FUSE_S_FAIL_IMMEDIATELY
>>> vstorage crutch is made under fc->lock, so the lock remains, and "FIXME" is there.
>>
>> There already fi->lock.
>>
>>> I think the lock is not needed, since the crutch's bit FUSE_S_FAIL_IMMEDIATELY
>>> is set under lock, but we do not need for all queued requests in fuse_invalidate_files()
>>> after the bit is set. So, I remain this logic, and you may decide whether you
>>> need the lock or not.
>>
>> Ok, Unfortunately loсk is needed here, but fc->bg_lock instead of
>> fc->lock to synchronize test_bit(FUSE_S_FAIL_IMMEDIATELY) and
>> fc->bg_queue. Possible race:
>>
> Your formatting lost all spaces.
>
One more try:
fuse_request_send_background():........................fuse_invalidate_files():
test_bit(FUSE_S_FAIL_IMMEDIATELY,.&ff->ff_state)
........................................................spin_lock(&fi->lock);
........................................................list_for_each_entry(ff,.&fi->rw_files,.rw_entry)
..........................................................set_bit(FUSE_S_FAIL_IMMEDIATELY,.&ff->ff_state);
........................................................spin_unlock(&fi->lock);
........................................................spin_lock(&fc->lock);
........................................................spin_lock(&fc->bg_lock);
........................................................fuse_kill_requests(fc,.inode,.&fc->bg_queue);
........................................................spin_unlock(&fc->bg_lock);
fuse_request_queue_background():
spin_lock(&fc->bg_lock);
list_add_tail(&req->list,.&fc->bg_queue);
flush_bg_queue(fc,.fiq);
spin_unlock(&fc->bg_lock);
........................................................spin_unlock(&fc->lock);
>> So, to fix that we need to have test_bit(FUSE_S_FAIL_IMMEDIATELY) and
>> list_add_tail(&req->list, &fc->bg_queue) together under one fc->bg_lock.
>>
>>
>>>>> BUG_ON(req->in.h.opcode != FUSE_READ);
>>>>> req->out.h.error = -EIO;
>>>>> __clear_bit(FR_BACKGROUND, &req->flags);
>>>>> __clear_bit(FR_PENDING, &req->flags);
>>>>> - spin_unlock(&fc->lock);
>>>>> request_end(fc, req);
>>>>> - } else if (fc->connected) {
>>>>> - fuse_request_send_background_nocheck(fc, req);
>>>>> - spin_unlock(&fc->lock);
>>>>> - } else {
>>>>> - spin_unlock(&fc->lock);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + if (!fuse_request_queue_background(fc, req)) {
>>>>> req->out.h.error = -ENOTCONN;
>>>>> req->end(fc, req);
>>>>> fuse_put_request(fc, req);
>>>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>>>> index edc314bd7156..6bffa1eef4dd 100644
>>>>> --- a/fs/fuse/file.c
>>>>> +++ b/fs/fuse/file.c
>>>>> @@ -1941,6 +1941,7 @@ __acquires(fc->lock)
>>>>> loff_t size = i_size_read(req->inode);
>>>>> struct fuse_write_in *inarg = &req->misc.write.in;
>>>>> __u64 data_size = req->num_pages * PAGE_CACHE_SIZE;
>>>>> + bool queued;
>>>>>
>>>>> if (!fc->connected ||
>>>>> test_bit(FUSE_S_FAIL_IMMEDIATELY, &req->ff->ff_state))
>>>>> @@ -1957,7 +1958,8 @@ __acquires(fc->lock)
>>>>>
>>>>> req->in.args[1].size = inarg->size;
>>>>> fi->writectr++;
>>>>> - fuse_request_send_background_nocheck(fc, req);
>>>>> + queued = fuse_request_queue_background(fc, req);
>>>>> + WARN_ON(!queued);
>>>>> return;
>>>>>
>>>>> out_free:
>>>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>>>> index a6eb566f8789..ee8b367f2956 100644
>>>>> --- a/fs/fuse/fuse_i.h
>>>>> +++ b/fs/fuse/fuse_i.h
>>>>> @@ -979,8 +979,7 @@ void fuse_request_check_and_send(struct fuse_conn *fc, struct fuse_req *req,
>>>>> */
>>>>> void fuse_request_send_background(struct fuse_conn *fc, struct fuse_req *req);
>>>>>
>>>>> -void fuse_request_send_background_nocheck(struct fuse_conn *fc,
>>>>> - struct fuse_req *req);
>>>>> +bool fuse_request_queue_background(struct fuse_conn *fc, struct fuse_req *req);
>>>>>
>>>>> /* Abort all requests */
>>>>> void fuse_abort_conn(struct fuse_conn *fc);
>>>>>
>>>
>
More information about the Devel
mailing list