From: Jeff Layton <jlayton@poochiereds.net> To: Al Viro <viro@ZenIV.linux.org.uk> Cc: bfields@fieldses.org, linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/4] fs: have flush_delayed_fput flush the workqueue job Date: Sun, 4 Oct 2015 09:35:53 -0400 [thread overview] Message-ID: <20151004093553.0ff25438@tlielax.poochiereds.net> (raw) In-Reply-To: <1442493587-32499-2-git-send-email-jeff.layton@primarydata.com> On Thu, 17 Sep 2015 08:39:44 -0400 Jeff Layton <jlayton@poochiereds.net> wrote: > I think there's a potential race in flush_delayed_fput. A kthread does > an fput() and that file gets added to the list and the delayed work is > scheduled. More than 1 jiffy passes, and the workqueue thread picks up > the work and starts running it. Then the kthread calls > flush_delayed_work. It sees that the list is empty and returns > immediately, even though the __fput for its file may not have run yet. > > Close this by making flush_delayed_fput use flush_delayed_work instead, > which should immediately schedule the work to run if it's not already, > and block until the workqueue job completes. > > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com> > --- > fs/file_table.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > It should be noted that the only current user of flush_delayed_fput() can call it before the workqueue threads are ever started. Looking at the code, I *think* this will still do the right thing -- block until those threads are started and then flush the work as usual, but do let me know if I've misread it. > diff --git a/fs/file_table.c b/fs/file_table.c > index ad17e05ebf95..52cc6803c07a 100644 > --- a/fs/file_table.c > +++ b/fs/file_table.c > @@ -244,6 +244,8 @@ static void ____fput(struct callback_head *work) > __fput(container_of(work, struct file, f_u.fu_rcuhead)); > } > > +static DECLARE_DELAYED_WORK(delayed_fput_work, delayed_fput); > + > /* > * If kernel thread really needs to have the final fput() it has done > * to complete, call this. The only user right now is the boot - we > @@ -256,11 +258,9 @@ static void ____fput(struct callback_head *work) > */ > void flush_delayed_fput(void) > { > - delayed_fput(NULL); > + flush_delayed_work(&delayed_fput_work); > } > > -static DECLARE_DELAYED_WORK(delayed_fput_work, delayed_fput); > - > void fput(struct file *file) > { > if (atomic_long_dec_and_test(&file->f_count)) { -- Jeff Layton <jlayton@poochiereds.net>
WARNING: multiple messages have this Message-ID (diff)
From: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> To: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org> Cc: bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH v2 1/4] fs: have flush_delayed_fput flush the workqueue job Date: Sun, 4 Oct 2015 09:35:53 -0400 [thread overview] Message-ID: <20151004093553.0ff25438@tlielax.poochiereds.net> (raw) In-Reply-To: <1442493587-32499-2-git-send-email-jeff.layton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org> On Thu, 17 Sep 2015 08:39:44 -0400 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> wrote: > I think there's a potential race in flush_delayed_fput. A kthread does > an fput() and that file gets added to the list and the delayed work is > scheduled. More than 1 jiffy passes, and the workqueue thread picks up > the work and starts running it. Then the kthread calls > flush_delayed_work. It sees that the list is empty and returns > immediately, even though the __fput for its file may not have run yet. > > Close this by making flush_delayed_fput use flush_delayed_work instead, > which should immediately schedule the work to run if it's not already, > and block until the workqueue job completes. > > Signed-off-by: Jeff Layton <jeff.layton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org> > --- > fs/file_table.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > It should be noted that the only current user of flush_delayed_fput() can call it before the workqueue threads are ever started. Looking at the code, I *think* this will still do the right thing -- block until those threads are started and then flush the work as usual, but do let me know if I've misread it. > diff --git a/fs/file_table.c b/fs/file_table.c > index ad17e05ebf95..52cc6803c07a 100644 > --- a/fs/file_table.c > +++ b/fs/file_table.c > @@ -244,6 +244,8 @@ static void ____fput(struct callback_head *work) > __fput(container_of(work, struct file, f_u.fu_rcuhead)); > } > > +static DECLARE_DELAYED_WORK(delayed_fput_work, delayed_fput); > + > /* > * If kernel thread really needs to have the final fput() it has done > * to complete, call this. The only user right now is the boot - we > @@ -256,11 +258,9 @@ static void ____fput(struct callback_head *work) > */ > void flush_delayed_fput(void) > { > - delayed_fput(NULL); > + flush_delayed_work(&delayed_fput_work); > } > > -static DECLARE_DELAYED_WORK(delayed_fput_work, delayed_fput); > - > void fput(struct file *file) > { > if (atomic_long_dec_and_test(&file->f_count)) { -- Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-10-04 13:35 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-09-17 12:39 [PATCH v2 0/4] fs: allow userland tasks to use delayed_fput infrastructure Jeff Layton 2015-09-17 12:39 ` Jeff Layton 2015-09-17 12:39 ` [PATCH v2 1/4] fs: have flush_delayed_fput flush the workqueue job Jeff Layton 2015-10-04 13:35 ` Jeff Layton [this message] 2015-10-04 13:35 ` Jeff Layton 2015-09-17 12:39 ` [PATCH v2 2/4] fs: add a kerneldoc header to fput Jeff Layton 2015-09-17 12:39 ` [PATCH v2 3/4] fs: add fput_queue Jeff Layton 2015-09-17 12:39 ` [PATCH v2 4/4] fs: export flush_delayed_fput Jeff Layton
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151004093553.0ff25438@tlielax.poochiereds.net \ --to=jlayton@poochiereds.net \ --cc=bfields@fieldses.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nfs@vger.kernel.org \ --cc=viro@ZenIV.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.