* [PATCH] ceph: fix sync read eof check deadlock
@ 2013-09-23 1:39 majianpeng
2013-09-24 1:32 ` Yan, Zheng
0 siblings, 1 reply; 4+ messages in thread
From: majianpeng @ 2013-09-23 1:39 UTC (permalink / raw)
To: sage; +Cc: Yan, Zheng, ceph-devel
As Yan,Zheng said, commit 0913444208db intruoduce a bug:"getattr need to
"read lock" inode's filelock. But the lock can be in unstable state.
the getattr request waits for lock's state to become stable, the lock
waits for client to release Fr cap."
Commit 6a026589ba333185c466c90 resolved the same bug also.
Before doing getattr, it must put the caps which already hold avoid
deadlock.
Reported-by: Yan, Zheng <zheng.z.yan@intel.com>
Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
---
fs/ceph/file.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 7da35c7..bc00ace 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -839,7 +839,15 @@ again:
ret = ceph_sync_read(iocb, &i, &checkeof);
if (checkeof && ret >= 0) {
- int statret = ceph_do_getattr(inode,
+ int statret;
+ /*
+ *Before getattr,it should put caps avoid
+ *deadlock.
+ */
+ ceph_put_cap_refs(ci, got);
+ got = 0;
+
+ statret = ceph_do_getattr(inode,
CEPH_STAT_CAP_SIZE);
/* hit EOF or hole? */
@@ -851,16 +859,23 @@ again:
read += ret;
checkeof = 0;
- goto again;
+ ret = ceph_get_caps(ci, CEPH_CAP_FILE_RD,
+ want, &got, -1);
+ if (ret < 0)
+ ret = 0;
+ else
+ goto again;
}
}
} else
ret = generic_file_aio_read(iocb, iov, nr_segs, pos);
- dout("aio_read %p %llx.%llx dropping cap refs on %s = %d\n",
- inode, ceph_vinop(inode), ceph_cap_string(got), (int)ret);
- ceph_put_cap_refs(ci, got);
+ if (got) {
+ dout("aio_read %p %llx.%llx dropping cap refs on %s = %d\n",
+ inode, ceph_vinop(inode), ceph_cap_string(got), (int)ret);
+ ceph_put_cap_refs(ci, got);
+ }
if (ret >= 0)
ret += read;
--
1.8.4-rc0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ceph: fix sync read eof check deadlock
2013-09-23 1:39 [PATCH] ceph: fix sync read eof check deadlock majianpeng
@ 2013-09-24 1:32 ` Yan, Zheng
2013-09-24 2:43 ` majianpeng
0 siblings, 1 reply; 4+ messages in thread
From: Yan, Zheng @ 2013-09-24 1:32 UTC (permalink / raw)
To: majianpeng; +Cc: sage, Yan, Zheng, ceph-devel
On Mon, Sep 23, 2013 at 9:39 AM, majianpeng <majianpeng@gmail.com> wrote:
> As Yan,Zheng said, commit 0913444208db intruoduce a bug:"getattr need to
> "read lock" inode's filelock. But the lock can be in unstable state.
> the getattr request waits for lock's state to become stable, the lock
> waits for client to release Fr cap."
>
> Commit 6a026589ba333185c466c90 resolved the same bug also.
> Before doing getattr, it must put the caps which already hold avoid
> deadlock.
>
> Reported-by: Yan, Zheng <zheng.z.yan@intel.com>
> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
> ---
> fs/ceph/file.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index 7da35c7..bc00ace 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -839,7 +839,15 @@ again:
> ret = ceph_sync_read(iocb, &i, &checkeof);
>
> if (checkeof && ret >= 0) {
> - int statret = ceph_do_getattr(inode,
> + int statret;
> + /*
> + *Before getattr,it should put caps avoid
> + *deadlock.
> + */
> + ceph_put_cap_refs(ci, got);
> + got = 0;
> +
> + statret = ceph_do_getattr(inode,
> CEPH_STAT_CAP_SIZE);
>
> /* hit EOF or hole? */
> @@ -851,16 +859,23 @@ again:
>
> read += ret;
> checkeof = 0;
> - goto again;
> + ret = ceph_get_caps(ci, CEPH_CAP_FILE_RD,
> + want, &got, -1);
> + if (ret < 0)
> + ret = 0;
> + else
> + goto again;
I think we should try getting Frc caps here, because mds may issue Fc
cap to the client while requesting the size.
Your previous patch moved the getattr code to here. please revert the
code to its original shape. I think it does the right thing.
Regards
Yan, Zheng
> }
> }
>
> } else
> ret = generic_file_aio_read(iocb, iov, nr_segs, pos);
>
> - dout("aio_read %p %llx.%llx dropping cap refs on %s = %d\n",
> - inode, ceph_vinop(inode), ceph_cap_string(got), (int)ret);
> - ceph_put_cap_refs(ci, got);
> + if (got) {
> + dout("aio_read %p %llx.%llx dropping cap refs on %s = %d\n",
> + inode, ceph_vinop(inode), ceph_cap_string(got), (int)ret);
> + ceph_put_cap_refs(ci, got);
> + }
>
> if (ret >= 0)
> ret += read;
> --
> 1.8.4-rc0
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH] ceph: fix sync read eof check deadlock
2013-09-24 1:32 ` Yan, Zheng
@ 2013-09-24 2:43 ` majianpeng
2013-09-24 5:29 ` Yan, Zheng
0 siblings, 1 reply; 4+ messages in thread
From: majianpeng @ 2013-09-24 2:43 UTC (permalink / raw)
To: Yan, Zheng; +Cc: sage, Yan, Zheng, ceph-devel
>On Mon, Sep 23, 2013 at 9:39 AM, majianpeng <majianpeng@gmail.com> wrote:
>> As Yan,Zheng said, commit 0913444208db intruoduce a bug:"getattr need to
>> "read lock" inode's filelock. But the lock can be in unstable state.
>> the getattr request waits for lock's state to become stable, the lock
>> waits for client to release Fr cap."
>>
>> Commit 6a026589ba333185c466c90 resolved the same bug also.
>> Before doing getattr, it must put the caps which already hold avoid
>> deadlock.
>>
>> Reported-by: Yan, Zheng <zheng.z.yan@intel.com>
>> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
>> ---
>> fs/ceph/file.c | 25 ++++++++++++++++++++-----
>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index 7da35c7..bc00ace 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>> @@ -839,7 +839,15 @@ again:
>> ret = ceph_sync_read(iocb, &i, &checkeof);
>>
>> if (checkeof && ret >= 0) {
>> - int statret = ceph_do_getattr(inode,
>> + int statret;
>> + /*
>> + *Before getattr,it should put caps avoid
>> + *deadlock.
>> + */
>> + ceph_put_cap_refs(ci, got);
>> + got = 0;
>> +
>> + statret = ceph_do_getattr(inode,
>> CEPH_STAT_CAP_SIZE);
>>
>> /* hit EOF or hole? */
>> @@ -851,16 +859,23 @@ again:
>>
>> read += ret;
>> checkeof = 0;
>> - goto again;
>> + ret = ceph_get_caps(ci, CEPH_CAP_FILE_RD,
>> + want, &got, -1);
>> + if (ret < 0)
>> + ret = 0;
>> + else
>> + goto again;
>
>I think we should try getting Frc caps here, because mds may issue Fc
>cap to the client while requesting the size.
>
Yes,it maybe get Fc.My though whether or not, the later read only using sync_mode.
The reason:
A:for requesting size only for sync-read not cache-read
B:By the original, it will make the cache-read to do the same thing which sync-read alread done.
Thanks!
Jianpeng Ma
>Your previous patch moved the getattr code to here. please revert the
>code to its original shape. I think it does the right thing.
>
>
>Regards
>Yan, Zheng
>
>
>> }
>> }
>>
>> } else
>> ret = generic_file_aio_read(iocb, iov, nr_segs, pos);
>>
>> - dout("aio_read %p %llx.%llx dropping cap refs on %s = %d\n",
>> - inode, ceph_vinop(inode), ceph_cap_string(got), (int)ret);
>> - ceph_put_cap_refs(ci, got);
>> + if (got) {
>> + dout("aio_read %p %llx.%llx dropping cap refs on %s = %d\n",
>> + inode, ceph_vinop(inode), ceph_cap_string(got), (int)ret);
>> + ceph_put_cap_refs(ci, got);
>> + }
>
>
>
>
>>
>> if (ret >= 0)
>> ret += read;
>> --
>> 1.8.4-rc0
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH] ceph: fix sync read eof check deadlock
2013-09-24 2:43 ` majianpeng
@ 2013-09-24 5:29 ` Yan, Zheng
0 siblings, 0 replies; 4+ messages in thread
From: Yan, Zheng @ 2013-09-24 5:29 UTC (permalink / raw)
To: majianpeng; +Cc: sage, Yan, Zheng, ceph-devel
On Tue, Sep 24, 2013 at 10:43 AM, majianpeng <majianpeng@gmail.com> wrote:
>>On Mon, Sep 23, 2013 at 9:39 AM, majianpeng <majianpeng@gmail.com> wrote:
>>> As Yan,Zheng said, commit 0913444208db intruoduce a bug:"getattr need to
>>> "read lock" inode's filelock. But the lock can be in unstable state.
>>> the getattr request waits for lock's state to become stable, the lock
>>> waits for client to release Fr cap."
>>>
>>> Commit 6a026589ba333185c466c90 resolved the same bug also.
>>> Before doing getattr, it must put the caps which already hold avoid
>>> deadlock.
>>>
>>> Reported-by: Yan, Zheng <zheng.z.yan@intel.com>
>>> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
>>> ---
>>> fs/ceph/file.c | 25 ++++++++++++++++++++-----
>>> 1 file changed, 20 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>>> index 7da35c7..bc00ace 100644
>>> --- a/fs/ceph/file.c
>>> +++ b/fs/ceph/file.c
>>> @@ -839,7 +839,15 @@ again:
>>> ret = ceph_sync_read(iocb, &i, &checkeof);
>>>
>>> if (checkeof && ret >= 0) {
>>> - int statret = ceph_do_getattr(inode,
>>> + int statret;
>>> + /*
>>> + *Before getattr,it should put caps avoid
>>> + *deadlock.
>>> + */
>>> + ceph_put_cap_refs(ci, got);
>>> + got = 0;
>>> +
>>> + statret = ceph_do_getattr(inode,
>>> CEPH_STAT_CAP_SIZE);
>>>
>>> /* hit EOF or hole? */
>>> @@ -851,16 +859,23 @@ again:
>>>
>>> read += ret;
>>> checkeof = 0;
>>> - goto again;
>>> + ret = ceph_get_caps(ci, CEPH_CAP_FILE_RD,
>>> + want, &got, -1);
>>> + if (ret < 0)
>>> + ret = 0;
>>> + else
>>> + goto again;
>>
>>I think we should try getting Frc caps here, because mds may issue Fc
>>cap to the client while requesting the size.
>>
> Yes,it maybe get Fc.My though whether or not, the later read only using sync_mode.
I don't see any reason for not doing buffered read when we succeed in
getting Fc cap. Buffered read is much faster than the sync read, we
should use it if possible.
> The reason:
> A:for requesting size only for sync-read not cache-read
> B:By the original, it will make the cache-read to do the same thing which sync-read alread done.
For buffered read, 'checkeof' be false, so the requesting size code
should never get executed.
>
> Thanks!
> Jianpeng Ma
>>Your previous patch moved the getattr code to here. please revert the
>>code to its original shape. I think it does the right thing.
>>
Another reason I prefer the original code is that it prints message
for getting/releasing caps.
Sage dropped your readv patch from the test branch (writev is still
in), please update and re-send the patch.
Regards
Yan, Zheng
>>
>>
>>> }
>>> }
>>>
>>> } else
>>> ret = generic_file_aio_read(iocb, iov, nr_segs, pos);
>>>
>>> - dout("aio_read %p %llx.%llx dropping cap refs on %s = %d\n",
>>> - inode, ceph_vinop(inode), ceph_cap_string(got), (int)ret);
>>> - ceph_put_cap_refs(ci, got);
>>> + if (got) {
>>> + dout("aio_read %p %llx.%llx dropping cap refs on %s = %d\n",
>>> + inode, ceph_vinop(inode), ceph_cap_string(got), (int)ret);
>>> + ceph_put_cap_refs(ci, got);
>>> + }
>>
>>
>>
>>
>>>
>>> if (ret >= 0)
>>> ret += read;
>>> --
>>> 1.8.4-rc0
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-24 5:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-23 1:39 [PATCH] ceph: fix sync read eof check deadlock majianpeng
2013-09-24 1:32 ` Yan, Zheng
2013-09-24 2:43 ` majianpeng
2013-09-24 5:29 ` Yan, Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.