Re: Re: [PATCH] ceph: fix sync read eof check deadlock

[majianpeng <majianpeng@gmail.com>]

>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