From: Jan Kara <jack@suse.cz>
To: Benjamin LaHaise <bcrl@kvack.org>
Cc: Jan Kara <jack@suse.cz>, Al Viro <viro@ZenIV.linux.org.uk>,
linux-aio@kvack.org, linux-fsdevel@vger.kernel.org,
Dmitry Monakhov <dmonakhov@openvz.org>
Subject: Re: [PATCH] aio: Fix freeze protection of aio writes
Date: Thu, 7 Jan 2016 16:15:52 +0100 [thread overview]
Message-ID: <20160107151552.GA9828@quack.suse.cz> (raw)
In-Reply-To: <20160107150705.GJ4439@kvack.org>
On Thu 07-01-16 10:07:05, Benjamin LaHaise wrote:
> On Thu, Jan 07, 2016 at 04:03:04PM +0100, Jan Kara wrote:
> > Currently we dropped freeze protection of aio writes just after IO was
> > submitted. Thus aio write could be in flight while the filesystem was
> > frozen and that could result in unexpected situation like aio completion
> > wanting to convert extent type on frozen filesystem. Testcase from
> > Dmitry triggering this is like:
> >
> > for ((i=0;i<60;i++));do fsfreeze -f /mnt ;sleep 1;fsfreeze -u /mnt;done &
> > fio --bs=4k --ioengine=libaio --iodepth=128 --size=1g --direct=1 \
> > --runtime=60 --filename=/mnt/file --name=rand-write --rw=randwrite
> >
> > Fix the problem by dropping freeze protection only once IO is completed
> > in aio_complete().
>
> Why isn't this code placed in file_start_write() and file_end_write()?
> That makes more sense to me than sprinkling it in the aio code.
Well, that would completely defeat the purpose of lockdep annotations for
fs freeze protection - we want lockdep to treat file_start_write() -
file_end_write() pair as a lock-unlock pair. But in case of AIO the unlock
will happen from a different process than lock and lockdep cannot handle
such cases. That's why for AIO we have to add some lockdep magic to avoid
false warnings.
Honza
> > Reported-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/aio.c | 31 ++++++++++++++++++++++++++++---
> > include/linux/fs.h | 1 +
> > 2 files changed, 29 insertions(+), 3 deletions(-)
> >
> > This patch seems to have fallen through the cracks. Al / Ben, can you please
> > merge it? Thanks!
> >
> > diff --git a/fs/aio.c b/fs/aio.c
> > index 155f84253f33..ee0871cb4677 100644
> > --- a/fs/aio.c
> > +++ b/fs/aio.c
> > @@ -1065,6 +1065,19 @@ static void aio_complete(struct kiocb *kiocb, long res, long res2)
> > unsigned tail, pos, head;
> > unsigned long flags;
> >
> > + if (kiocb->ki_flags & IOCB_WRITE) {
> > + struct file *f = kiocb->ki_filp;
> > +
> > + /*
> > + * Tell lockdep we inherited freeze protection from submission
> > + * thread.
> > + */
> > + percpu_rwsem_acquire(
> > + &f->f_inode->i_sb->s_writers.rw_sem[SB_FREEZE_WRITE-1],
> > + 1, _THIS_IP_);
> > + file_end_write(f);
> > + }
> > +
> > /*
> > * Special case handling for sync iocbs:
> > * - events go directly into the iocb for fast handling
> > @@ -1449,13 +1462,25 @@ rw_common:
> >
> > len = ret;
> >
> > - if (rw == WRITE)
> > + if (rw == WRITE) {
> > file_start_write(file);
> > + req->ki_flags |= IOCB_WRITE;
> > + }
> >
> > ret = iter_op(req, &iter);
> >
> > - if (rw == WRITE)
> > - file_end_write(file);
> > + if (rw == WRITE) {
> > + /*
> > + * We release freeze protection in aio_complete(). Fool
> > + * lockdep by telling it the lock got released so that
> > + * it doesn't complain about held lock when we return
> > + * to userspace.
> > + */
> > + percpu_rwsem_release(
> > + &file->f_inode->i_sb->s_writers.rw_sem[SB_FREEZE_WRITE-1],
> > + 1, _THIS_IP_);
> > + }
> > +
> > kfree(iovec);
> > break;
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3aa514254161..54af40ed6a26 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -319,6 +319,7 @@ struct writeback_control;
> > #define IOCB_EVENTFD (1 << 0)
> > #define IOCB_APPEND (1 << 1)
> > #define IOCB_DIRECT (1 << 2)
> > +#define IOCB_WRITE (1 << 3)
> >
> > struct kiocb {
> > struct file *ki_filp;
> > --
> > 2.6.2
>
> --
> "Thought is the essence of where you are now."
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2016-01-07 15:15 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-07 15:03 [PATCH] aio: Fix freeze protection of aio writes Jan Kara
2016-01-07 15:07 ` Benjamin LaHaise
2016-01-07 15:08 ` Benjamin LaHaise
2016-01-07 15:15 ` Jan Kara [this message]
2016-01-07 16:24 ` Benjamin LaHaise
-- strict thread matches above, loose matches on Subject: below --
2015-11-26 17:10 Jan Kara
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=20160107151552.GA9828@quack.suse.cz \
--to=jack@suse.cz \
--cc=bcrl@kvack.org \
--cc=dmonakhov@openvz.org \
--cc=linux-aio@kvack.org \
--cc=linux-fsdevel@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).