Function ceph_check_delayed_caps() is called from the mdsc->delayed_work
workqueue and it can be kept looping for quite some time if caps keep being
added back to the mdsc->cap_delay_list. This may result in the watchdog
tainting the kernel with the softlockup flag.
This patch re-arranges the loop through the caps list so that it initially
removes all the caps from list, adding them to a temporary list. And then, with
less locking contention, it will eventually call the ceph_check_caps() for each
inode. Any caps added to the list in the meantime will be handled in the next
run.
Cc: [email protected]
Signed-off-by: Luis Henriques <[email protected]>
---
Hi Jeff!
So, I've not based this patch on top of your patchset that gets rid of
ceph_async_iput() so that it will make it easier to backport it for stable
kernels. Of course I'm not 100% this classifies as stable material.
Other than that, I've been testing this patch and I couldn't see anything
breaking. Let me know what you think.
(I *think* I've seen a tracker bug for this in the past but I couldn't
find it. I guess it could be added as a 'Link:' tag.)
Cheers,
--
Luis
fs/ceph/caps.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index a5e93b185515..727e41e3b939 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -4229,6 +4229,7 @@ void ceph_check_delayed_caps(struct ceph_mds_client *mdsc)
{
struct inode *inode;
struct ceph_inode_info *ci;
+ LIST_HEAD(caps_list);
dout("check_delayed_caps\n");
spin_lock(&mdsc->cap_delay_lock);
@@ -4239,19 +4240,23 @@ void ceph_check_delayed_caps(struct ceph_mds_client *mdsc)
if ((ci->i_ceph_flags & CEPH_I_FLUSH) == 0 &&
time_before(jiffies, ci->i_hold_caps_max))
break;
- list_del_init(&ci->i_cap_delay_list);
+ list_move_tail(&ci->i_cap_delay_list, &caps_list);
+ }
+ spin_unlock(&mdsc->cap_delay_lock);
+ while (!list_empty(&caps_list)) {
+ ci = list_first_entry(&caps_list,
+ struct ceph_inode_info,
+ i_cap_delay_list);
+ list_del_init(&ci->i_cap_delay_list);
inode = igrab(&ci->vfs_inode);
if (inode) {
- spin_unlock(&mdsc->cap_delay_lock);
dout("check_delayed_caps on %p\n", inode);
ceph_check_caps(ci, 0, NULL);
/* avoid calling iput_final() in tick thread */
ceph_async_iput(inode);
- spin_lock(&mdsc->cap_delay_lock);
}
}
- spin_unlock(&mdsc->cap_delay_lock);
}
/*
On Fri, 2021-06-25 at 16:45 +0100, Luis Henriques wrote:
> Function ceph_check_delayed_caps() is called from the mdsc->delayed_work
> workqueue and it can be kept looping for quite some time if caps keep being
> added back to the mdsc->cap_delay_list. This may result in the watchdog
> tainting the kernel with the softlockup flag.
>
> This patch re-arranges the loop through the caps list so that it initially
> removes all the caps from list, adding them to a temporary list. And then, with
> less locking contention, it will eventually call the ceph_check_caps() for each
> inode. Any caps added to the list in the meantime will be handled in the next
> run.
>
> Cc: [email protected]
> Signed-off-by: Luis Henriques <[email protected]>
> ---
> Hi Jeff!
>
> So, I've not based this patch on top of your patchset that gets rid of
> ceph_async_iput() so that it will make it easier to backport it for stable
> kernels. Of course I'm not 100% this classifies as stable material.
>
> Other than that, I've been testing this patch and I couldn't see anything
> breaking. Let me know what you think.
>
> (I *think* I've seen a tracker bug for this in the past but I couldn't
> find it. I guess it could be added as a 'Link:' tag.)
>
> Cheers,
> --
> Luis
>
> fs/ceph/caps.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index a5e93b185515..727e41e3b939 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4229,6 +4229,7 @@ void ceph_check_delayed_caps(struct ceph_mds_client *mdsc)
> {
> struct inode *inode;
> struct ceph_inode_info *ci;
> + LIST_HEAD(caps_list);
>
> dout("check_delayed_caps\n");
> spin_lock(&mdsc->cap_delay_lock);
> @@ -4239,19 +4240,23 @@ void ceph_check_delayed_caps(struct ceph_mds_client *mdsc)
> if ((ci->i_ceph_flags & CEPH_I_FLUSH) == 0 &&
> time_before(jiffies, ci->i_hold_caps_max))
> break;
> - list_del_init(&ci->i_cap_delay_list);
> + list_move_tail(&ci->i_cap_delay_list, &caps_list);
> + }
> + spin_unlock(&mdsc->cap_delay_lock);
>
> + while (!list_empty(&caps_list)) {
> + ci = list_first_entry(&caps_list,
> + struct ceph_inode_info,
> + i_cap_delay_list);
> + list_del_init(&ci->i_cap_delay_list);
> inode = igrab(&ci->vfs_inode);
> if (inode) {
> - spin_unlock(&mdsc->cap_delay_lock);
> dout("check_delayed_caps on %p\n", inode);
> ceph_check_caps(ci, 0, NULL);
> /* avoid calling iput_final() in tick thread */
> ceph_async_iput(inode);
> - spin_lock(&mdsc->cap_delay_lock);
> }
> }
> - spin_unlock(&mdsc->cap_delay_lock);
> }
>
> /*
I'm not sure this approach is viable, unfortunately. Once you've dropped
the cap_delay_lock, then nothing protects the i_cap_delay_list head
anymore.
So you could detach these objects and put them on the private list, and
then once you drop the spinlock another task could find one of them and
(e.g.) call __cap_delay_requeue on it, potentially corrupting your list.
I think we'll need to come up with a different way to do this...
--
Jeff Layton <[email protected]>
On Fri, Jun 25, 2021 at 12:54:44PM -0400, Jeff Layton wrote:
<...>
> I'm not sure this approach is viable, unfortunately. Once you've dropped
> the cap_delay_lock, then nothing protects the i_cap_delay_list head
> anymore.
>
> So you could detach these objects and put them on the private list, and
> then once you drop the spinlock another task could find one of them and
> (e.g.) call __cap_delay_requeue on it, potentially corrupting your list.
>
> I think we'll need to come up with a different way to do this...
Ugh, yeah I see what you mean.
Another option I can think off is to time-bound this loop, so that it
would stop after finding the first ci->i_hold_caps_max timestamp that was
set *after* the start of the current run. I'll see if I can come up with
an RFC shortly.
Cheers,
--
Lu?s
On Mon, 2021-06-28 at 10:04 +0100, Luis Henriques wrote:
> On Fri, Jun 25, 2021 at 12:54:44PM -0400, Jeff Layton wrote:
> <...>
> > I'm not sure this approach is viable, unfortunately. Once you've dropped
> > the cap_delay_lock, then nothing protects the i_cap_delay_list head
> > anymore.
> >
> > So you could detach these objects and put them on the private list, and
> > then once you drop the spinlock another task could find one of them and
> > (e.g.) call __cap_delay_requeue on it, potentially corrupting your list.
> >
> > I think we'll need to come up with a different way to do this...
>
> Ugh, yeah I see what you mean.
>
> Another option I can think off is to time-bound this loop, so that it
> would stop after finding the first ci->i_hold_caps_max timestamp that was
> set *after* the start of the current run. I'll see if I can come up with
> an RFC shortly.
>
Sounds like a reasonable thing to do.
The catch there is that those caps may end up being delayed up to 5s
more than they would have, since schedule_delayed always uses a 5s
delay. That delay could be made more dynamic if it becomes an issue.
Maybe have the schedule_delayed callers calculate and pass in a timeout
and schedule the next run for that point in the future? Then
delayed_work could schedule the next run to coincide with the timeout of
the next entry on the list.
--
Jeff Layton <[email protected]>