[2/5] Btrfs: treat RWF_{,D}SYNC writes as sync for CRCs
diff mbox series

Message ID ba7aa871e255c0e264a782b863513b9afd499f91.1565900769.git.osandov@fb.com
State New
Headers show
Series
  • Btrfs: add interface for writing compressed extent directly
Related show

Commit Message

Omar Sandoval Aug. 15, 2019, 9:04 p.m. UTC
From: Omar Sandoval <osandov@fb.com>

In btrfs_file_write_iter(), we treat a write as synchrononous if the
file is marked as synchronous. However, with pwritev2(), a write with
RWF_SYNC or RWF_DSYNC is also synchronous even if the file isn't by
default. Make sure we bump the sync_writers counter in that case, too,
so that we'll do the CRCs synchronously.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Josef Bacik Aug. 16, 2019, 4:59 p.m. UTC | #1
On Thu, Aug 15, 2019 at 02:04:03PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> In btrfs_file_write_iter(), we treat a write as synchrononous if the
> file is marked as synchronous. However, with pwritev2(), a write with
> RWF_SYNC or RWF_DSYNC is also synchronous even if the file isn't by
> default. Make sure we bump the sync_writers counter in that case, too,
> so that we'll do the CRCs synchronously.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef
David Sterba Aug. 27, 2019, 12:35 p.m. UTC | #2
On Thu, Aug 15, 2019 at 02:04:03PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> In btrfs_file_write_iter(), we treat a write as synchrononous if the
> file is marked as synchronous. However, with pwritev2(), a write with
> RWF_SYNC or RWF_DSYNC is also synchronous even if the file isn't by
> default. Make sure we bump the sync_writers counter in that case, too,
> so that we'll do the CRCs synchronously.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  fs/btrfs/file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 4393b6b24e02..27223753da7b 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1882,7 +1882,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
>  	u64 start_pos;
>  	u64 end_pos;
>  	ssize_t num_written = 0;
> -	bool sync = (file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host);
> +	bool sync = iocb->ki_flags & IOCB_DSYNC;

I'd like to merge the patches 1-3, but have hard time matching the
changelog to the change here. It's from one set of sync flags to
another, mentioning pwritev2 but that's a syscall and the function
itself does not use the sync flags at all. That's probably somewhere
deep in the vfs calls but that's what I'd appreciate stated explicitly
in the changelog as I was not able to find it out in a reasonable time.
Omar Sandoval Aug. 27, 2019, 5:44 p.m. UTC | #3
On Tue, Aug 27, 2019 at 02:35:13PM +0200, David Sterba wrote:
> On Thu, Aug 15, 2019 at 02:04:03PM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > In btrfs_file_write_iter(), we treat a write as synchrononous if the
> > file is marked as synchronous. However, with pwritev2(), a write with
> > RWF_SYNC or RWF_DSYNC is also synchronous even if the file isn't by
> > default. Make sure we bump the sync_writers counter in that case, too,
> > so that we'll do the CRCs synchronously.
> > 
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  fs/btrfs/file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 4393b6b24e02..27223753da7b 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1882,7 +1882,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
> >  	u64 start_pos;
> >  	u64 end_pos;
> >  	ssize_t num_written = 0;
> > -	bool sync = (file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host);
> > +	bool sync = iocb->ki_flags & IOCB_DSYNC;
> 
> I'd like to merge the patches 1-3, but have hard time matching the
> changelog to the change here. It's from one set of sync flags to
> another, mentioning pwritev2 but that's a syscall and the function
> itself does not use the sync flags at all. That's probably somewhere
> deep in the vfs calls but that's what I'd appreciate stated explicitly
> in the changelog as I was not able to find it out in a reasonable time.

You're right, there are a few layers here. How about this for the
changelog:


The VFS indicates a synchronous write to ->write_iter() via
iocb->ki_flags. The IOCB_{,D}SYNC flags may be set based on the file
(see iocb_flags()) or the RWF_* flags passed to a syscall like
pwritev2() (see kiocb_set_rw_flags()). However, in
btrfs_file_write_iter(), we're checking if a write is synchronous based
only on the file; we use this to decide when to bump the sync_writers
counter and thus do CRCs synchronously. Make sure we do this for all
synchronous writes as determined by the VFS.


Let me know if you want me to resend with the new changelog.
David Sterba Aug. 27, 2019, 6:16 p.m. UTC | #4
On Tue, Aug 27, 2019 at 10:44:39AM -0700, Omar Sandoval wrote:
> On Tue, Aug 27, 2019 at 02:35:13PM +0200, David Sterba wrote:
> > On Thu, Aug 15, 2019 at 02:04:03PM -0700, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > > 
> > > In btrfs_file_write_iter(), we treat a write as synchrononous if the
> > > file is marked as synchronous. However, with pwritev2(), a write with
> > > RWF_SYNC or RWF_DSYNC is also synchronous even if the file isn't by
> > > default. Make sure we bump the sync_writers counter in that case, too,
> > > so that we'll do the CRCs synchronously.
> > > 
> > > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > > ---
> > >  fs/btrfs/file.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > > index 4393b6b24e02..27223753da7b 100644
> > > --- a/fs/btrfs/file.c
> > > +++ b/fs/btrfs/file.c
> > > @@ -1882,7 +1882,7 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
> > >  	u64 start_pos;
> > >  	u64 end_pos;
> > >  	ssize_t num_written = 0;
> > > -	bool sync = (file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host);
> > > +	bool sync = iocb->ki_flags & IOCB_DSYNC;
> > 
> > I'd like to merge the patches 1-3, but have hard time matching the
> > changelog to the change here. It's from one set of sync flags to
> > another, mentioning pwritev2 but that's a syscall and the function
> > itself does not use the sync flags at all. That's probably somewhere
> > deep in the vfs calls but that's what I'd appreciate stated explicitly
> > in the changelog as I was not able to find it out in a reasonable time.
> 
> You're right, there are a few layers here. How about this for the
> changelog:
> 
> 
> The VFS indicates a synchronous write to ->write_iter() via
> iocb->ki_flags. The IOCB_{,D}SYNC flags may be set based on the file
> (see iocb_flags()) or the RWF_* flags passed to a syscall like
> pwritev2() (see kiocb_set_rw_flags()). However, in
> btrfs_file_write_iter(), we're checking if a write is synchronous based
> only on the file; we use this to decide when to bump the sync_writers
> counter and thus do CRCs synchronously. Make sure we do this for all
> synchronous writes as determined by the VFS.

That's great, thanks, no need to resend.

Patch
diff mbox series

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 4393b6b24e02..27223753da7b 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1882,7 +1882,7 @@  static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	u64 start_pos;
 	u64 end_pos;
 	ssize_t num_written = 0;
-	bool sync = (file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host);
+	bool sync = iocb->ki_flags & IOCB_DSYNC;
 	ssize_t err;
 	loff_t pos;
 	size_t count;