Re: [PATCH 4/4] iomap: Use FUA for pure data O_DSYNC DIO writes
On Tue, Apr 24, 2018 at 10:34:44AM -0700, Christoph Hellwig wrote:
> On Sat, Apr 21, 2018 at 02:54:05PM +0200, Jan Kara wrote:
> > > - if (iocb->ki_flags & IOCB_DSYNC)
> > > + if (iocb->ki_flags & IOCB_DSYNC) {
> > > dio->flags |= IOMAP_DIO_NEED_SYNC;
> > > + /*
> > > + * We optimistically try using FUA for this IO. Any
> > > + * non-FUA write that occurs will clear this flag, hence
> > > + * we know before completion whether a cache flush is
> > > + * necessary.
> > > + */
> > > + dio->flags |= IOMAP_DIO_WRITE_FUA;
> > > + }
> >
> > So I don't think this is quite correct. IOCB_DSYNC gets set also for O_SYNC
> > writes (in that case we also set IOCB_SYNC). And for those we cannot use
> > the FUA optimization AFAICT (definitely IOMAP_F_DIRTY isn't a safe
> > indicator of a need of full fsync for O_SYNC). Other than that the patch
> > looks good to me.
>
> Oops, good catch. I think the above if should just be
>
> if (iocb->ki_flags & (IOCB_DSYNC | IOCB_SYNC) == IOCB_DSYNC)) {
>
> and we are fine.
Ah, not exactly. IOMAP_DIO_NEED_SYNC needs to be set for either
DYSNC or SYNC writes, while IOMAP_DIO_WRITE_FUA should only be set
for DSYNC.
I'll fix this up appropriately.
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
Re: [PATCH 4/4] iomap: Use FUA for pure data O_DSYNC DIO writes
On Wed 25-04-18 00:07:07, Holger Hoffstätte wrote:
> On 04/24/18 19:34, Christoph Hellwig wrote:
> > On Sat, Apr 21, 2018 at 02:54:05PM +0200, Jan Kara wrote:
> > > > - if (iocb->ki_flags & IOCB_DSYNC)
> > > > + if (iocb->ki_flags & IOCB_DSYNC) {
> > > > dio->flags |= IOMAP_DIO_NEED_SYNC;
> > > > + /*
> > > > +* We optimistically try using FUA for this IO.
> > > > Any
> > > > +* non-FUA write that occurs will clear this
> > > > flag, hence
> > > > +* we know before completion whether a cache
> > > > flush is
> > > > +* necessary.
> > > > +*/
> > > > + dio->flags |= IOMAP_DIO_WRITE_FUA;
> > > > + }
> > >
> > > So I don't think this is quite correct. IOCB_DSYNC gets set also for
> > > O_SYNC
> > > writes (in that case we also set IOCB_SYNC). And for those we cannot use
> > > the FUA optimization AFAICT (definitely IOMAP_F_DIRTY isn't a safe
> > > indicator of a need of full fsync for O_SYNC). Other than that the patch
> > > looks good to me.
> >
> > Oops, good catch. I think the above if should just be
> >
> > if (iocb->ki_flags & (IOCB_DSYNC | IOCB_SYNC) == IOCB_DSYNC)) {
> >
> > and we are fine.
>
> The above line just gives parenthesis salad errors, so why not compromise
> on:
>
> if ((iocb->ki_flags & (IOCB_DSYNC | IOCB_SYNC)) == IOCB_DSYNC) {
>
> Unless my bit twiddling has completely left me I think this is what was
> intended, and it actually compiles too.
Yup, I agree this is what needs to happen.
Honza
--
Jan Kara
SUSE Labs, CR
Re: [PATCH 4/4] iomap: Use FUA for pure data O_DSYNC DIO writes
On Wed, Apr 25, 2018 at 12:07:07AM +0200, Holger Hoffstätte wrote:
> The above line just gives parenthesis salad errors, so why not compromise
> on:
>
> if ((iocb->ki_flags & (IOCB_DSYNC | IOCB_SYNC)) == IOCB_DSYNC) {
>
> Unless my bit twiddling has completely left me I think this is what was
> intended, and it actually compiles too.
Yes, that is what was intended :)
Re: [PATCH 4/4] iomap: Use FUA for pure data O_DSYNC DIO writes
On 04/24/18 19:34, Christoph Hellwig wrote:
On Sat, Apr 21, 2018 at 02:54:05PM +0200, Jan Kara wrote:
- if (iocb->ki_flags & IOCB_DSYNC)
+ if (iocb->ki_flags & IOCB_DSYNC) {
dio->flags |= IOMAP_DIO_NEED_SYNC;
+ /*
+* We optimistically try using FUA for this IO. Any
+* non-FUA write that occurs will clear this flag, hence
+* we know before completion whether a cache flush is
+* necessary.
+*/
+ dio->flags |= IOMAP_DIO_WRITE_FUA;
+ }
So I don't think this is quite correct. IOCB_DSYNC gets set also for O_SYNC
writes (in that case we also set IOCB_SYNC). And for those we cannot use
the FUA optimization AFAICT (definitely IOMAP_F_DIRTY isn't a safe
indicator of a need of full fsync for O_SYNC). Other than that the patch
looks good to me.
Oops, good catch. I think the above if should just be
if (iocb->ki_flags & (IOCB_DSYNC | IOCB_SYNC) == IOCB_DSYNC)) {
and we are fine.
The above line just gives parenthesis salad errors, so why not compromise
on:
if ((iocb->ki_flags & (IOCB_DSYNC | IOCB_SYNC)) == IOCB_DSYNC) {
Unless my bit twiddling has completely left me I think this is what was
intended, and it actually compiles too.
cheers
Holger
Re: [PATCH 4/4] iomap: Use FUA for pure data O_DSYNC DIO writes
On Sat, Apr 21, 2018 at 02:54:05PM +0200, Jan Kara wrote:
> > - if (iocb->ki_flags & IOCB_DSYNC)
> > + if (iocb->ki_flags & IOCB_DSYNC) {
> > dio->flags |= IOMAP_DIO_NEED_SYNC;
> > + /*
> > +* We optimistically try using FUA for this IO. Any
> > +* non-FUA write that occurs will clear this flag, hence
> > +* we know before completion whether a cache flush is
> > +* necessary.
> > +*/
> > + dio->flags |= IOMAP_DIO_WRITE_FUA;
> > + }
>
> So I don't think this is quite correct. IOCB_DSYNC gets set also for O_SYNC
> writes (in that case we also set IOCB_SYNC). And for those we cannot use
> the FUA optimization AFAICT (definitely IOMAP_F_DIRTY isn't a safe
> indicator of a need of full fsync for O_SYNC). Other than that the patch
> looks good to me.
Oops, good catch. I think the above if should just be
if (iocb->ki_flags & (IOCB_DSYNC | IOCB_SYNC) == IOCB_DSYNC)) {
and we are fine.
>
> Honza
> --
> Jan Kara
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
---end quoted text---
Re: [PATCH 4/4] iomap: Use FUA for pure data O_DSYNC DIO writes
On Wed 18-04-18 14:08:28, Dave Chinner wrote:
> @@ -1012,8 +1035,16 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
> dio->flags |= IOMAP_DIO_DIRTY;
> } else {
> dio->flags |= IOMAP_DIO_WRITE;
> - if (iocb->ki_flags & IOCB_DSYNC)
> + if (iocb->ki_flags & IOCB_DSYNC) {
> dio->flags |= IOMAP_DIO_NEED_SYNC;
> + /*
> + * We optimistically try using FUA for this IO. Any
> + * non-FUA write that occurs will clear this flag, hence
> + * we know before completion whether a cache flush is
> + * necessary.
> + */
> + dio->flags |= IOMAP_DIO_WRITE_FUA;
> + }
So I don't think this is quite correct. IOCB_DSYNC gets set also for O_SYNC
writes (in that case we also set IOCB_SYNC). And for those we cannot use
the FUA optimization AFAICT (definitely IOMAP_F_DIRTY isn't a safe
indicator of a need of full fsync for O_SYNC). Other than that the patch
looks good to me.
Honza
--
Jan Kara
SUSE Labs, CR
