* Treat REQ_FUA and REQ_PREFLUSH as synchronous
@ 2017-05-02 10:21 Jan Kara
2017-05-02 14:45 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2017-05-02 10:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-block, linux-fsdevel
Hello,
Commit b685d3d65ac7 "block: treat REQ_FUA and REQ_PREFLUSH as synchronous"
made requests with REQ_FUA and REQ_PREFLUSH to be treated as synchronous
and dropped REQ_SYNC from definitions of WRITE_FUA and friends. This
however introduced a bunch of bugs to filesystems (I know about ext4,
btrfs, f2fs, gfs2, reiserfs regressing because of this) because they
implicitely expected REQ_FUA or REQ_PREFLUSH implies a synchronous request.
At the first sight they do however generic_make_request_checks() will strip
REQ_FUA and REQ_PREFLUSH flags from a bio if the underlying storage does
not have volatile write cache and so writes suddenly become async. I will
go and fix filesystems to explicitely set REQ_SYNC if they want sync
behavior as that is a good cleanup anyway but I wanted to question whether
it makes sense to treat REQ_FUA and REQ_PREFLUSH ops as synchronous in
op_is_sync() since callers cannot rely on this anyway... Thoughts?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Treat REQ_FUA and REQ_PREFLUSH as synchronous
2017-05-02 10:21 Treat REQ_FUA and REQ_PREFLUSH as synchronous Jan Kara
@ 2017-05-02 14:45 ` Christoph Hellwig
2017-05-02 14:49 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2017-05-02 14:45 UTC (permalink / raw)
To: Jan Kara; +Cc: Christoph Hellwig, linux-block, linux-fsdevel
On Tue, May 02, 2017 at 12:21:23PM +0200, Jan Kara wrote:
> it makes sense to treat REQ_FUA and REQ_PREFLUSH ops as synchronous in
> op_is_sync() since callers cannot rely on this anyway... Thoughts?
I'm fine with treating them as sync.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Treat REQ_FUA and REQ_PREFLUSH as synchronous
2017-05-02 14:45 ` Christoph Hellwig
@ 2017-05-02 14:49 ` Jens Axboe
2017-05-02 15:15 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2017-05-02 14:49 UTC (permalink / raw)
To: Christoph Hellwig, Jan Kara; +Cc: linux-block, linux-fsdevel
On 05/02/2017 08:45 AM, Christoph Hellwig wrote:
> On Tue, May 02, 2017 at 12:21:23PM +0200, Jan Kara wrote:
>> it makes sense to treat REQ_FUA and REQ_PREFLUSH ops as synchronous in
>> op_is_sync() since callers cannot rely on this anyway... Thoughts?
>
> I'm fine with treating them as sync.
Yes me too. It makes sense to do so implicitly, and it avoids having
to go and add REQ_SYNC in a bunch of places. That will also be more
error proof in the future.
Jan, will you send a patch for that?
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Treat REQ_FUA and REQ_PREFLUSH as synchronous
2017-05-02 14:49 ` Jens Axboe
@ 2017-05-02 15:15 ` Jan Kara
2017-05-02 15:18 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2017-05-02 15:15 UTC (permalink / raw)
To: Jens Axboe; +Cc: Christoph Hellwig, Jan Kara, linux-block, linux-fsdevel
On Tue 02-05-17 08:49:14, Jens Axboe wrote:
> On 05/02/2017 08:45 AM, Christoph Hellwig wrote:
> > On Tue, May 02, 2017 at 12:21:23PM +0200, Jan Kara wrote:
> >> it makes sense to treat REQ_FUA and REQ_PREFLUSH ops as synchronous in
> >> op_is_sync() since callers cannot rely on this anyway... Thoughts?
> >
> > I'm fine with treating them as sync.
>
> Yes me too. It makes sense to do so implicitly, and it avoids having
> to go and add REQ_SYNC in a bunch of places. That will also be more
> error proof in the future.
>
> Jan, will you send a patch for that?
Hum, so I've just sent out fixes to individual filesystems to set REQ_SYNC
explicitely :-|. So would you prefer to do something like:
if (op_is_flush(bio->bi_opf) &&
!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
+ bio->bi_opf |= REQ_SYNC;
if (!nr_sectors) {
err = 0;
goto end_io;
}
}
in generic_make_request_checks()? Or just set it unconditionally in that
function if we see REQ_FUA | REQ_PREFLUSH set? Because we cannot just add
REQ_SYNC into REQ_FUA and REQ_PREFLUSH definitions as it used to be with
WRITE_FUA & co. definitions...
In the end I think that modifying filesystems (and drivers/md) to set REQ_SYNC
was the cleanest way to go as much as it was annoying...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Treat REQ_FUA and REQ_PREFLUSH as synchronous
2017-05-02 15:15 ` Jan Kara
@ 2017-05-02 15:18 ` Christoph Hellwig
2017-05-02 16:29 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2017-05-02 15:18 UTC (permalink / raw)
To: Jan Kara; +Cc: Jens Axboe, Christoph Hellwig, linux-block, linux-fsdevel
On Tue, May 02, 2017 at 05:15:58PM +0200, Jan Kara wrote:
> in generic_make_request_checks()? Or just set it unconditionally in that
> function if we see REQ_FUA | REQ_PREFLUSH set?
Just add REQ_FUA and REQ_PREFLUSH to the test in op_is_sync.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Treat REQ_FUA and REQ_PREFLUSH as synchronous
2017-05-02 15:18 ` Christoph Hellwig
@ 2017-05-02 16:29 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2017-05-02 16:29 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jan Kara, Jens Axboe, linux-block, linux-fsdevel
On Tue 02-05-17 08:18:15, Christoph Hellwig wrote:
> On Tue, May 02, 2017 at 05:15:58PM +0200, Jan Kara wrote:
> > in generic_make_request_checks()? Or just set it unconditionally in that
> > function if we see REQ_FUA | REQ_PREFLUSH set?
>
> Just add REQ_FUA and REQ_PREFLUSH to the test in op_is_sync.
It is there. The problem happens when REQ_FUA and REQ_PREFLUSH get stripped
from bio because the underlying storage doesn't have volatile cache, the
bio suddently becomes treated as async which regresses performance.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-02 16:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-02 10:21 Treat REQ_FUA and REQ_PREFLUSH as synchronous Jan Kara
2017-05-02 14:45 ` Christoph Hellwig
2017-05-02 14:49 ` Jens Axboe
2017-05-02 15:15 ` Jan Kara
2017-05-02 15:18 ` Christoph Hellwig
2017-05-02 16:29 ` Jan Kara
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).