[PATCH] ceph: wait on async create before checking caps for syncfs

[PATCH] ceph: wait on async create before checking caps for syncfs

[Jeff Layton]

Currently, we'll call ceph_check_caps, but if we're still waiting on the
reply, we'll end up spinning around on the same inode in
flush_dirty_session_caps. Wait for the async create reply before
flushing caps.

Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
URL: https://tracker.ceph.com/issues/55823
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/caps.c | 1 +
 1 file changed, 1 insertion(+)

I don't know if this will fix the tx queue stalls completely, but I
haven't seen one with this patch in place. I think it makes sense on its
own, either way.

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 0a48bf829671..5ecfff4b37c9 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
 		ihold(inode);
 		dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
 		spin_unlock(&mdsc->cap_dirty_lock);
+		ceph_wait_on_async_create(inode);
 		ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
 		iput(inode);
 		spin_lock(&mdsc->cap_dirty_lock);
-- 
2.36.1

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Xiubo Li]

On 6/7/22 7:31 AM, Jeff Layton wrote:
> Currently, we'll call ceph_check_caps, but if we're still waiting on the
> reply, we'll end up spinning around on the same inode in
> flush_dirty_session_caps. Wait for the async create reply before
> flushing caps.
>
> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> URL: https://tracker.ceph.com/issues/55823
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>   fs/ceph/caps.c | 1 +
>   1 file changed, 1 insertion(+)
>
> I don't know if this will fix the tx queue stalls completely, but I
> haven't seen one with this patch in place. I think it makes sense on its
> own, either way.
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 0a48bf829671..5ecfff4b37c9 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
>   		ihold(inode);
>   		dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
>   		spin_unlock(&mdsc->cap_dirty_lock);
> +		ceph_wait_on_async_create(inode);
>   		ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
>   		iput(inode);
>   		spin_lock(&mdsc->cap_dirty_lock);

This looks good.

Possibly we can add one dedicated list to store the async creating 
inodes instead of getting stuck all the others ?

-- Xiubo

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Jeff Layton]

On Tue, 2022-06-07 at 09:11 +0800, Xiubo Li wrote:
> On 6/7/22 7:31 AM, Jeff Layton wrote:
> > Currently, we'll call ceph_check_caps, but if we're still waiting on the
> > reply, we'll end up spinning around on the same inode in
> > flush_dirty_session_caps. Wait for the async create reply before
> > flushing caps.
> > 
> > Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> > URL: https://tracker.ceph.com/issues/55823
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >   fs/ceph/caps.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > I don't know if this will fix the tx queue stalls completely, but I
> > haven't seen one with this patch in place. I think it makes sense on its
> > own, either way.
> > 
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index 0a48bf829671..5ecfff4b37c9 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
> >   		ihold(inode);
> >   		dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
> >   		spin_unlock(&mdsc->cap_dirty_lock);
> > +		ceph_wait_on_async_create(inode);
> >   		ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
> >   		iput(inode);
> >   		spin_lock(&mdsc->cap_dirty_lock);
> 
> This looks good.
> 
> Possibly we can add one dedicated list to store the async creating 
> inodes instead of getting stuck all the others ?
> 

I'd be open to that. I think we ought to take this patch first to fix
the immediate bug though, before we add extra complexity.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Xiubo Li]

On 6/7/22 9:21 AM, Jeff Layton wrote:
> On Tue, 2022-06-07 at 09:11 +0800, Xiubo Li wrote:
>> On 6/7/22 7:31 AM, Jeff Layton wrote:
>>> Currently, we'll call ceph_check_caps, but if we're still waiting on the
>>> reply, we'll end up spinning around on the same inode in
>>> flush_dirty_session_caps. Wait for the async create reply before
>>> flushing caps.
>>>
>>> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
>>> URL: https://tracker.ceph.com/issues/55823
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>    fs/ceph/caps.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> I don't know if this will fix the tx queue stalls completely, but I
>>> haven't seen one with this patch in place. I think it makes sense on its
>>> own, either way.
>>>
>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>>> index 0a48bf829671..5ecfff4b37c9 100644
>>> --- a/fs/ceph/caps.c
>>> +++ b/fs/ceph/caps.c
>>> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
>>>    		ihold(inode);
>>>    		dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
>>>    		spin_unlock(&mdsc->cap_dirty_lock);
>>> +		ceph_wait_on_async_create(inode);
>>>    		ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
>>>    		iput(inode);
>>>    		spin_lock(&mdsc->cap_dirty_lock);
>> This looks good.
>>
>> Possibly we can add one dedicated list to store the async creating
>> inodes instead of getting stuck all the others ?
>>
> I'd be open to that. I think we ought to take this patch first to fix
> the immediate bug though, before we add extra complexity.

Sounds good to me.

I will merge it to the testing branch for now and let's improve it later.

Thanks

-- Xiubo

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Jeff Layton]

On Tue, 2022-06-07 at 09:50 +0800, Xiubo Li wrote:
> On 6/7/22 9:21 AM, Jeff Layton wrote:
> > On Tue, 2022-06-07 at 09:11 +0800, Xiubo Li wrote:
> > > On 6/7/22 7:31 AM, Jeff Layton wrote:
> > > > Currently, we'll call ceph_check_caps, but if we're still waiting on the
> > > > reply, we'll end up spinning around on the same inode in
> > > > flush_dirty_session_caps. Wait for the async create reply before
> > > > flushing caps.
> > > > 
> > > > Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> > > > URL: https://tracker.ceph.com/issues/55823
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >    fs/ceph/caps.c | 1 +
> > > >    1 file changed, 1 insertion(+)
> > > > 
> > > > I don't know if this will fix the tx queue stalls completely, but I
> > > > haven't seen one with this patch in place. I think it makes sense on its
> > > > own, either way.
> > > > 
> > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > > index 0a48bf829671..5ecfff4b37c9 100644
> > > > --- a/fs/ceph/caps.c
> > > > +++ b/fs/ceph/caps.c
> > > > @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
> > > >    		ihold(inode);
> > > >    		dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
> > > >    		spin_unlock(&mdsc->cap_dirty_lock);
> > > > +		ceph_wait_on_async_create(inode);
> > > >    		ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
> > > >    		iput(inode);
> > > >    		spin_lock(&mdsc->cap_dirty_lock);
> > > This looks good.
> > > 
> > > Possibly we can add one dedicated list to store the async creating
> > > inodes instead of getting stuck all the others ?
> > > 
> > I'd be open to that. I think we ought to take this patch first to fix
> > the immediate bug though, before we add extra complexity.
> 
> Sounds good to me.
> 
> I will merge it to the testing branch for now and let's improve it later.
> 

Can we also mark this for stable? It's a pretty nasty bug, potentially.
We should get this into mainline fairly soon.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Xiubo Li]

On 6/8/22 9:58 PM, Jeff Layton wrote:
> On Tue, 2022-06-07 at 09:50 +0800, Xiubo Li wrote:
>> On 6/7/22 9:21 AM, Jeff Layton wrote:
>>> On Tue, 2022-06-07 at 09:11 +0800, Xiubo Li wrote:
>>>> On 6/7/22 7:31 AM, Jeff Layton wrote:
>>>>> Currently, we'll call ceph_check_caps, but if we're still waiting on the
>>>>> reply, we'll end up spinning around on the same inode in
>>>>> flush_dirty_session_caps. Wait for the async create reply before
>>>>> flushing caps.
>>>>>
>>>>> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
>>>>> URL: https://tracker.ceph.com/issues/55823
>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>> ---
>>>>>     fs/ceph/caps.c | 1 +
>>>>>     1 file changed, 1 insertion(+)
>>>>>
>>>>> I don't know if this will fix the tx queue stalls completely, but I
>>>>> haven't seen one with this patch in place. I think it makes sense on its
>>>>> own, either way.
>>>>>
>>>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>>>>> index 0a48bf829671..5ecfff4b37c9 100644
>>>>> --- a/fs/ceph/caps.c
>>>>> +++ b/fs/ceph/caps.c
>>>>> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
>>>>>     		ihold(inode);
>>>>>     		dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
>>>>>     		spin_unlock(&mdsc->cap_dirty_lock);
>>>>> +		ceph_wait_on_async_create(inode);
>>>>>     		ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
>>>>>     		iput(inode);
>>>>>     		spin_lock(&mdsc->cap_dirty_lock);
>>>> This looks good.
>>>>
>>>> Possibly we can add one dedicated list to store the async creating
>>>> inodes instead of getting stuck all the others ?
>>>>
>>> I'd be open to that. I think we ought to take this patch first to fix
>>> the immediate bug though, before we add extra complexity.
>> Sounds good to me.
>>
>> I will merge it to the testing branch for now and let's improve it later.
>>
> Can we also mark this for stable? It's a pretty nasty bug, potentially.
> We should get this into mainline fairly soon.
Yeah, sure.
>
> Thanks,

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Yan, Zheng]

The recent series of patches that add "wait on async xxxx" at various
places do not seem correct. The correct fix should make mds avoid any
wait when handling async requests.


On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> Currently, we'll call ceph_check_caps, but if we're still waiting on the
> reply, we'll end up spinning around on the same inode in
> flush_dirty_session_caps. Wait for the async create reply before
> flushing caps.
>
> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> URL: https://tracker.ceph.com/issues/55823
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/caps.c | 1 +
>  1 file changed, 1 insertion(+)
>
> I don't know if this will fix the tx queue stalls completely, but I
> haven't seen one with this patch in place. I think it makes sense on its
> own, either way.
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 0a48bf829671..5ecfff4b37c9 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
>                 ihold(inode);
>                 dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
>                 spin_unlock(&mdsc->cap_dirty_lock);
> +               ceph_wait_on_async_create(inode);
>                 ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
>                 iput(inode);
>                 spin_lock(&mdsc->cap_dirty_lock);
> --
> 2.36.1
>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Xiubo Li]

On 6/9/22 10:15 AM, Yan, Zheng wrote:
> The recent series of patches that add "wait on async xxxx" at various
> places do not seem correct. The correct fix should make mds avoid any
> wait when handling async requests.
>
In this case I am thinking what will happen if the async create request 
is deferred, then the cap flush related request should fail to find the 
ino.

Should we wait ? Then how to distinguish from migrating a subtree and a 
deferred async create cases ?


> On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
>> Currently, we'll call ceph_check_caps, but if we're still waiting on the
>> reply, we'll end up spinning around on the same inode in
>> flush_dirty_session_caps. Wait for the async create reply before
>> flushing caps.
>>
>> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
>> URL: https://tracker.ceph.com/issues/55823
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>>   fs/ceph/caps.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> I don't know if this will fix the tx queue stalls completely, but I
>> haven't seen one with this patch in place. I think it makes sense on its
>> own, either way.
>>
>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>> index 0a48bf829671..5ecfff4b37c9 100644
>> --- a/fs/ceph/caps.c
>> +++ b/fs/ceph/caps.c
>> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
>>                  ihold(inode);
>>                  dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
>>                  spin_unlock(&mdsc->cap_dirty_lock);
>> +               ceph_wait_on_async_create(inode);
>>                  ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
>>                  iput(inode);
>>                  spin_lock(&mdsc->cap_dirty_lock);
>> --
>> 2.36.1
>>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Yan, Zheng]

On Thu, Jun 9, 2022 at 11:19 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 6/9/22 10:15 AM, Yan, Zheng wrote:
> > The recent series of patches that add "wait on async xxxx" at various
> > places do not seem correct. The correct fix should make mds avoid any
> > wait when handling async requests.
> >
> In this case I am thinking what will happen if the async create request
> is deferred, then the cap flush related request should fail to find the
> ino.
>
> Should we wait ? Then how to distinguish from migrating a subtree and a
> deferred async create cases ?
>

async op caps are revoked at freezingtree stage of subtree migration.
see Locker::invalidate_lock_caches

>
> > On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
> >> Currently, we'll call ceph_check_caps, but if we're still waiting on the
> >> reply, we'll end up spinning around on the same inode in
> >> flush_dirty_session_caps. Wait for the async create reply before
> >> flushing caps.
> >>
> >> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> >> URL: https://tracker.ceph.com/issues/55823
> >> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >> ---
> >>   fs/ceph/caps.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> I don't know if this will fix the tx queue stalls completely, but I
> >> haven't seen one with this patch in place. I think it makes sense on its
> >> own, either way.
> >>
> >> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> >> index 0a48bf829671..5ecfff4b37c9 100644
> >> --- a/fs/ceph/caps.c
> >> +++ b/fs/ceph/caps.c
> >> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
> >>                  ihold(inode);
> >>                  dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
> >>                  spin_unlock(&mdsc->cap_dirty_lock);
> >> +               ceph_wait_on_async_create(inode);
> >>                  ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
> >>                  iput(inode);
> >>                  spin_lock(&mdsc->cap_dirty_lock);
> >> --
> >> 2.36.1
> >>
>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Xiubo Li]

On 6/9/22 11:29 AM, Yan, Zheng wrote:
> On Thu, Jun 9, 2022 at 11:19 AM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 6/9/22 10:15 AM, Yan, Zheng wrote:
>>> The recent series of patches that add "wait on async xxxx" at various
>>> places do not seem correct. The correct fix should make mds avoid any
>>> wait when handling async requests.
>>>
>> In this case I am thinking what will happen if the async create request
>> is deferred, then the cap flush related request should fail to find the
>> ino.
>>
>> Should we wait ? Then how to distinguish from migrating a subtree and a
>> deferred async create cases ?
>>
> async op caps are revoked at freezingtree stage of subtree migration.
> see Locker::invalidate_lock_caches
>
Sorry I may not totally understand this issue.

I think you mean in case of migration and then the MDS will revoke caps 
for the async create files and then the kclient will send a MclientCap 
request to mds, right ?

If my understanding is correct, there is another case that:

1, async create a fileA

2, then write a lot of data to it and then release the Fw cap ref, and 
if we should report the size to MDS, it will send a MclientCap request 
to MDS too.

3, what if the async create of fileA was deferred due to some reason, 
then the MclientCap request will fail to find the ino ?


>>> On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
>>>> Currently, we'll call ceph_check_caps, but if we're still waiting on the
>>>> reply, we'll end up spinning around on the same inode in
>>>> flush_dirty_session_caps. Wait for the async create reply before
>>>> flushing caps.
>>>>
>>>> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
>>>> URL: https://tracker.ceph.com/issues/55823
>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>> ---
>>>>    fs/ceph/caps.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> I don't know if this will fix the tx queue stalls completely, but I
>>>> haven't seen one with this patch in place. I think it makes sense on its
>>>> own, either way.
>>>>
>>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>>>> index 0a48bf829671..5ecfff4b37c9 100644
>>>> --- a/fs/ceph/caps.c
>>>> +++ b/fs/ceph/caps.c
>>>> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
>>>>                   ihold(inode);
>>>>                   dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
>>>>                   spin_unlock(&mdsc->cap_dirty_lock);
>>>> +               ceph_wait_on_async_create(inode);
>>>>                   ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
>>>>                   iput(inode);
>>>>                   spin_lock(&mdsc->cap_dirty_lock);
>>>> --
>>>> 2.36.1
>>>>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Yan, Zheng]

On Thu, Jun 9, 2022 at 11:56 AM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 6/9/22 11:29 AM, Yan, Zheng wrote:
> > On Thu, Jun 9, 2022 at 11:19 AM Xiubo Li <xiubli@redhat.com> wrote:
> >>
> >> On 6/9/22 10:15 AM, Yan, Zheng wrote:
> >>> The recent series of patches that add "wait on async xxxx" at various
> >>> places do not seem correct. The correct fix should make mds avoid any
> >>> wait when handling async requests.
> >>>
> >> In this case I am thinking what will happen if the async create request
> >> is deferred, then the cap flush related request should fail to find the
> >> ino.
> >>
> >> Should we wait ? Then how to distinguish from migrating a subtree and a
> >> deferred async create cases ?
> >>
> > async op caps are revoked at freezingtree stage of subtree migration.
> > see Locker::invalidate_lock_caches
> >
> Sorry I may not totally understand this issue.
>
> I think you mean in case of migration and then the MDS will revoke caps
> for the async create files and then the kclient will send a MclientCap
> request to mds, right ?
>
> If my understanding is correct, there is another case that:
>
> 1, async create a fileA
>
> 2, then write a lot of data to it and then release the Fw cap ref, and
> if we should report the size to MDS, it will send a MclientCap request
> to MDS too.
>
> 3, what if the async create of fileA was deferred due to some reason,
> then the MclientCap request will fail to find the ino ?
>

Async op should not be deferred in any case.

>
> >>> On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
> >>>> Currently, we'll call ceph_check_caps, but if we're still waiting on the
> >>>> reply, we'll end up spinning around on the same inode in
> >>>> flush_dirty_session_caps. Wait for the async create reply before
> >>>> flushing caps.
> >>>>
> >>>> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> >>>> URL: https://tracker.ceph.com/issues/55823
> >>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >>>> ---
> >>>>    fs/ceph/caps.c | 1 +
> >>>>    1 file changed, 1 insertion(+)
> >>>>
> >>>> I don't know if this will fix the tx queue stalls completely, but I
> >>>> haven't seen one with this patch in place. I think it makes sense on its
> >>>> own, either way.
> >>>>
> >>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> >>>> index 0a48bf829671..5ecfff4b37c9 100644
> >>>> --- a/fs/ceph/caps.c
> >>>> +++ b/fs/ceph/caps.c
> >>>> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
> >>>>                   ihold(inode);
> >>>>                   dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
> >>>>                   spin_unlock(&mdsc->cap_dirty_lock);
> >>>> +               ceph_wait_on_async_create(inode);
> >>>>                   ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
> >>>>                   iput(inode);
> >>>>                   spin_lock(&mdsc->cap_dirty_lock);
> >>>> --
> >>>> 2.36.1
> >>>>
>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Xiubo Li]

On 6/9/22 12:02 PM, Yan, Zheng wrote:
> On Thu, Jun 9, 2022 at 11:56 AM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 6/9/22 11:29 AM, Yan, Zheng wrote:
>>> On Thu, Jun 9, 2022 at 11:19 AM Xiubo Li <xiubli@redhat.com> wrote:
>>>> On 6/9/22 10:15 AM, Yan, Zheng wrote:
>>>>> The recent series of patches that add "wait on async xxxx" at various
>>>>> places do not seem correct. The correct fix should make mds avoid any
>>>>> wait when handling async requests.
>>>>>
>>>> In this case I am thinking what will happen if the async create request
>>>> is deferred, then the cap flush related request should fail to find the
>>>> ino.
>>>>
>>>> Should we wait ? Then how to distinguish from migrating a subtree and a
>>>> deferred async create cases ?
>>>>
>>> async op caps are revoked at freezingtree stage of subtree migration.
>>> see Locker::invalidate_lock_caches
>>>
>> Sorry I may not totally understand this issue.
>>
>> I think you mean in case of migration and then the MDS will revoke caps
>> for the async create files and then the kclient will send a MclientCap
>> request to mds, right ?
>>
>> If my understanding is correct, there is another case that:
>>
>> 1, async create a fileA
>>
>> 2, then write a lot of data to it and then release the Fw cap ref, and
>> if we should report the size to MDS, it will send a MclientCap request
>> to MDS too.
>>
>> 3, what if the async create of fileA was deferred due to some reason,
>> then the MclientCap request will fail to find the ino ?
>>
> Async op should not be deferred in any case.

I am still checking the 'mdcache->path_traverse()', in which it seems 
could forward the request or requeue the request when failing to acquire 
locks. And also in case [1].

[1] https://github.com/ceph/ceph/blob/main/src/mds/Server.cc#L4501.


>>>>> On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
>>>>>> Currently, we'll call ceph_check_caps, but if we're still waiting on the
>>>>>> reply, we'll end up spinning around on the same inode in
>>>>>> flush_dirty_session_caps. Wait for the async create reply before
>>>>>> flushing caps.
>>>>>>
>>>>>> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
>>>>>> URL: https://tracker.ceph.com/issues/55823
>>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>>> ---
>>>>>>     fs/ceph/caps.c | 1 +
>>>>>>     1 file changed, 1 insertion(+)
>>>>>>
>>>>>> I don't know if this will fix the tx queue stalls completely, but I
>>>>>> haven't seen one with this patch in place. I think it makes sense on its
>>>>>> own, either way.
>>>>>>
>>>>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>>>>>> index 0a48bf829671..5ecfff4b37c9 100644
>>>>>> --- a/fs/ceph/caps.c
>>>>>> +++ b/fs/ceph/caps.c
>>>>>> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
>>>>>>                    ihold(inode);
>>>>>>                    dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
>>>>>>                    spin_unlock(&mdsc->cap_dirty_lock);
>>>>>> +               ceph_wait_on_async_create(inode);
>>>>>>                    ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
>>>>>>                    iput(inode);
>>>>>>                    spin_lock(&mdsc->cap_dirty_lock);
>>>>>> --
>>>>>> 2.36.1
>>>>>>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Xiubo Li]

On 6/9/22 12:02 PM, Yan, Zheng wrote:
> On Thu, Jun 9, 2022 at 11:56 AM Xiubo Li <xiubli@redhat.com> wrote:
>>
>> On 6/9/22 11:29 AM, Yan, Zheng wrote:
>>> On Thu, Jun 9, 2022 at 11:19 AM Xiubo Li <xiubli@redhat.com> wrote:
>>>> On 6/9/22 10:15 AM, Yan, Zheng wrote:
>>>>> The recent series of patches that add "wait on async xxxx" at various
>>>>> places do not seem correct. The correct fix should make mds avoid any
>>>>> wait when handling async requests.
>>>>>
>>>> In this case I am thinking what will happen if the async create request
>>>> is deferred, then the cap flush related request should fail to find the
>>>> ino.
>>>>
>>>> Should we wait ? Then how to distinguish from migrating a subtree and a
>>>> deferred async create cases ?
>>>>
>>> async op caps are revoked at freezingtree stage of subtree migration.
>>> see Locker::invalidate_lock_caches
>>>
>> Sorry I may not totally understand this issue.
>>
>> I think you mean in case of migration and then the MDS will revoke caps
>> for the async create files and then the kclient will send a MclientCap
>> request to mds, right ?
>>
>> If my understanding is correct, there is another case that:
>>
>> 1, async create a fileA
>>
>> 2, then write a lot of data to it and then release the Fw cap ref, and
>> if we should report the size to MDS, it will send a MclientCap request
>> to MDS too.
>>
>> 3, what if the async create of fileA was deferred due to some reason,
>> then the MclientCap request will fail to find the ino ?
>>
> Async op should not be deferred in any case.

Recently we have hit a similar bug, caused by deferring a request and 
requeuing it and the following request was executed before it.


>>>>> On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
>>>>>> Currently, we'll call ceph_check_caps, but if we're still waiting on the
>>>>>> reply, we'll end up spinning around on the same inode in
>>>>>> flush_dirty_session_caps. Wait for the async create reply before
>>>>>> flushing caps.
>>>>>>
>>>>>> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
>>>>>> URL: https://tracker.ceph.com/issues/55823
>>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>>> ---
>>>>>>     fs/ceph/caps.c | 1 +
>>>>>>     1 file changed, 1 insertion(+)
>>>>>>
>>>>>> I don't know if this will fix the tx queue stalls completely, but I
>>>>>> haven't seen one with this patch in place. I think it makes sense on its
>>>>>> own, either way.
>>>>>>
>>>>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>>>>>> index 0a48bf829671..5ecfff4b37c9 100644
>>>>>> --- a/fs/ceph/caps.c
>>>>>> +++ b/fs/ceph/caps.c
>>>>>> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
>>>>>>                    ihold(inode);
>>>>>>                    dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
>>>>>>                    spin_unlock(&mdsc->cap_dirty_lock);
>>>>>> +               ceph_wait_on_async_create(inode);
>>>>>>                    ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
>>>>>>                    iput(inode);
>>>>>>                    spin_lock(&mdsc->cap_dirty_lock);
>>>>>> --
>>>>>> 2.36.1
>>>>>>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Yan, Zheng]

On Thu, Jun 9, 2022 at 12:18 PM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 6/9/22 12:02 PM, Yan, Zheng wrote:
> > On Thu, Jun 9, 2022 at 11:56 AM Xiubo Li <xiubli@redhat.com> wrote:
> >>
> >> On 6/9/22 11:29 AM, Yan, Zheng wrote:
> >>> On Thu, Jun 9, 2022 at 11:19 AM Xiubo Li <xiubli@redhat.com> wrote:
> >>>> On 6/9/22 10:15 AM, Yan, Zheng wrote:
> >>>>> The recent series of patches that add "wait on async xxxx" at various
> >>>>> places do not seem correct. The correct fix should make mds avoid any
> >>>>> wait when handling async requests.
> >>>>>
> >>>> In this case I am thinking what will happen if the async create request
> >>>> is deferred, then the cap flush related request should fail to find the
> >>>> ino.
> >>>>
> >>>> Should we wait ? Then how to distinguish from migrating a subtree and a
> >>>> deferred async create cases ?
> >>>>
> >>> async op caps are revoked at freezingtree stage of subtree migration.
> >>> see Locker::invalidate_lock_caches
> >>>
> >> Sorry I may not totally understand this issue.
> >>
> >> I think you mean in case of migration and then the MDS will revoke caps
> >> for the async create files and then the kclient will send a MclientCap
> >> request to mds, right ?
> >>
> >> If my understanding is correct, there is another case that:
> >>
> >> 1, async create a fileA
> >>
> >> 2, then write a lot of data to it and then release the Fw cap ref, and
> >> if we should report the size to MDS, it will send a MclientCap request
> >> to MDS too.
> >>
> >> 3, what if the async create of fileA was deferred due to some reason,
> >> then the MclientCap request will fail to find the ino ?
> >>
> > Async op should not be deferred in any case.
>
> Recently we have hit a similar bug, caused by deferring a request and
> requeuing it and the following request was executed before it.
>
that bug is mds bug.

> >>>>> On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
> >>>>>> Currently, we'll call ceph_check_caps, but if we're still waiting on the
> >>>>>> reply, we'll end up spinning around on the same inode in
> >>>>>> flush_dirty_session_caps. Wait for the async create reply before
> >>>>>> flushing caps.
> >>>>>>
> >>>>>> Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> >>>>>> URL: https://tracker.ceph.com/issues/55823
> >>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >>>>>> ---
> >>>>>>     fs/ceph/caps.c | 1 +
> >>>>>>     1 file changed, 1 insertion(+)
> >>>>>>
> >>>>>> I don't know if this will fix the tx queue stalls completely, but I
> >>>>>> haven't seen one with this patch in place. I think it makes sense on its
> >>>>>> own, either way.
> >>>>>>
> >>>>>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> >>>>>> index 0a48bf829671..5ecfff4b37c9 100644
> >>>>>> --- a/fs/ceph/caps.c
> >>>>>> +++ b/fs/ceph/caps.c
> >>>>>> @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
> >>>>>>                    ihold(inode);
> >>>>>>                    dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
> >>>>>>                    spin_unlock(&mdsc->cap_dirty_lock);
> >>>>>> +               ceph_wait_on_async_create(inode);
> >>>>>>                    ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
> >>>>>>                    iput(inode);
> >>>>>>                    spin_lock(&mdsc->cap_dirty_lock);
> >>>>>> --
> >>>>>> 2.36.1
> >>>>>>
>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Jeff Layton]

Are you suggesting that the MDS ought to hold a cap message for an inode
before its create request is processed? Note that the MDS won't even be
aware that the inode even _exists_ at that point. As far as the MDS
knows, it's just be a delegated inode number to the client. At what
point does the MDS give up on holding such a cap request if the create
request never comes in for some reason?

I don't see the harm in making the client wait until it gets a create
reply before sending a cap message. If we want to revert fbed7045f552
instead, we can do that, but it'll cause a regression until the MDS is
fixed [1]. Regardless, we need to either take this patch or revert that
one. 

I move that we take this patch for now to address the softlockups. Once
the MDS is fixed we could revert this and fbed7045f552 without causing a
regression.

[1]: https://tracker.ceph.com/issues/54107


On Thu, 2022-06-09 at 10:15 +0800, Yan, Zheng wrote:
> The recent series of patches that add "wait on async xxxx" at various
> places do not seem correct. The correct fix should make mds avoid any
> wait when handling async requests.
> 
> 
> On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > Currently, we'll call ceph_check_caps, but if we're still waiting on the
> > reply, we'll end up spinning around on the same inode in
> > flush_dirty_session_caps. Wait for the async create reply before
> > flushing caps.
> > 
> > Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> > URL: https://tracker.ceph.com/issues/55823
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/caps.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > I don't know if this will fix the tx queue stalls completely, but I
> > haven't seen one with this patch in place. I think it makes sense on its
> > own, either way.
> > 
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index 0a48bf829671..5ecfff4b37c9 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
> >                 ihold(inode);
> >                 dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
> >                 spin_unlock(&mdsc->cap_dirty_lock);
> > +               ceph_wait_on_async_create(inode);
> >                 ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
> >                 iput(inode);
> >                 spin_lock(&mdsc->cap_dirty_lock);
> > --
> > 2.36.1
> > 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Yan, Zheng]

On Wed, Jun 29, 2022 at 8:08 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> Are you suggesting that the MDS ought to hold a cap message for an inode
> before its create request is processed? Note that the MDS won't even be
> aware that the inode even _exists_ at that point. As far as the MDS
> knows, it's just be a delegated inode number to the client. At what
> point does the MDS give up on holding such a cap request if the create
> request never comes in for some reason?
>
For an async request, MDS should not process it immediately.  If there
is any wait when handling async request, it's mds bug. I suggest
tracking down any wait, and fix it.


> I don't see the harm in making the client wait until it gets a create
> reply before sending a cap message. If we want to revert fbed7045f552
> instead, we can do that, but it'll cause a regression until the MDS is
> fixed [1]. Regardless, we need to either take this patch or revert that
> one.
>
> I move that we take this patch for now to address the softlockups. Once
> the MDS is fixed we could revert this and fbed7045f552 without causing a
> regression.
>
> [1]: https://tracker.ceph.com/issues/54107
>
>
> On Thu, 2022-06-09 at 10:15 +0800, Yan, Zheng wrote:
> > The recent series of patches that add "wait on async xxxx" at various
> > places do not seem correct. The correct fix should make mds avoid any
> > wait when handling async requests.
> >
> >
> > On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > Currently, we'll call ceph_check_caps, but if we're still waiting on the
> > > reply, we'll end up spinning around on the same inode in
> > > flush_dirty_session_caps. Wait for the async create reply before
> > > flushing caps.
> > >
> > > Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> > > URL: https://tracker.ceph.com/issues/55823
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/caps.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > I don't know if this will fix the tx queue stalls completely, but I
> > > haven't seen one with this patch in place. I think it makes sense on its
> > > own, either way.
> > >
> > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > index 0a48bf829671..5ecfff4b37c9 100644
> > > --- a/fs/ceph/caps.c
> > > +++ b/fs/ceph/caps.c
> > > @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
> > >                 ihold(inode);
> > >                 dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
> > >                 spin_unlock(&mdsc->cap_dirty_lock);
> > > +               ceph_wait_on_async_create(inode);
> > >                 ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
> > >                 iput(inode);
> > >                 spin_lock(&mdsc->cap_dirty_lock);
> > > --
> > > 2.36.1
> > >
>
> --
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: wait on async create before checking caps for syncfs

[Yan, Zheng]

On Thu, Jun 30, 2022 at 10:33 AM Yan, Zheng <ukernel@gmail.com> wrote:
>
> On Wed, Jun 29, 2022 at 8:08 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > Are you suggesting that the MDS ought to hold a cap message for an inode
> > before its create request is processed? Note that the MDS won't even be
> > aware that the inode even _exists_ at that point. As far as the MDS
> > knows, it's just be a delegated inode number to the client. At what
> > point does the MDS give up on holding such a cap request if the create
> > request never comes in for some reason?
> >
> For an async request, MDS should not process it immediately.  If there
> is any wait when handling async request, it's mds bug. I suggest
> tracking down any wait, and fix it.
>

I mean: For an async request, MDS should process it immediately. This
is important for preserving request ordering.

>
> > I don't see the harm in making the client wait until it gets a create
> > reply before sending a cap message. If we want to revert fbed7045f552
> > instead, we can do that, but it'll cause a regression until the MDS is
> > fixed [1]. Regardless, we need to either take this patch or revert that
> > one.
> >
> > I move that we take this patch for now to address the softlockups. Once
> > the MDS is fixed we could revert this and fbed7045f552 without causing a
> > regression.
> >
> > [1]: https://tracker.ceph.com/issues/54107
> >
> >
> > On Thu, 2022-06-09 at 10:15 +0800, Yan, Zheng wrote:
> > > The recent series of patches that add "wait on async xxxx" at various
> > > places do not seem correct. The correct fix should make mds avoid any
> > > wait when handling async requests.
> > >
> > >
> > > On Wed, Jun 8, 2022 at 12:56 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > >
> > > > Currently, we'll call ceph_check_caps, but if we're still waiting on the
> > > > reply, we'll end up spinning around on the same inode in
> > > > flush_dirty_session_caps. Wait for the async create reply before
> > > > flushing caps.
> > > >
> > > > Fixes: fbed7045f552 (ceph: wait for async create reply before sending any cap messages)
> > > > URL: https://tracker.ceph.com/issues/55823
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/ceph/caps.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > I don't know if this will fix the tx queue stalls completely, but I
> > > > haven't seen one with this patch in place. I think it makes sense on its
> > > > own, either way.
> > > >
> > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > > index 0a48bf829671..5ecfff4b37c9 100644
> > > > --- a/fs/ceph/caps.c
> > > > +++ b/fs/ceph/caps.c
> > > > @@ -4389,6 +4389,7 @@ static void flush_dirty_session_caps(struct ceph_mds_session *s)
> > > >                 ihold(inode);
> > > >                 dout("flush_dirty_caps %llx.%llx\n", ceph_vinop(inode));
> > > >                 spin_unlock(&mdsc->cap_dirty_lock);
> > > > +               ceph_wait_on_async_create(inode);
> > > >                 ceph_check_caps(ci, CHECK_CAPS_FLUSH, NULL);
> > > >                 iput(inode);
> > > >                 spin_lock(&mdsc->cap_dirty_lock);
> > > > --
> > > > 2.36.1
> > > >
> >
> > --
> > Jeff Layton <jlayton@kernel.org>