diff mbox series

[07/15] btrfs: Move FS error state bit early during write

Message ID 20200921144353.31319-8-rgoldwyn@suse.de
State New
Headers show
Series BTRFS DIO inode locking/D_SYNC fix | expand

Commit Message

Goldwyn Rodrigues Sept. 21, 2020, 2:43 p.m. UTC
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

fs_info->fs_state is a filesystem bit check as opposed to inode
and can be performed before we begin with write checks. This eliminates
inode lock/unlock in case of error bit is set.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/file.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

Comments

Josef Bacik Sept. 22, 2020, 2:38 p.m. UTC | #1
On 9/21/20 10:43 AM, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> fs_info->fs_state is a filesystem bit check as opposed to inode
> and can be performed before we begin with write checks. This eliminates
> inode lock/unlock in case of error bit is set.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

The check was there because remove_file_privs may have tripped the error 
starting a trans handle.  That being said we could easily have been set before 
now, so this is fine

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

Thanks,

Josef
Nikolay Borisov Sept. 23, 2020, 9:10 a.m. UTC | #2
On 21.09.20 г. 17:43 ч., Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> fs_info->fs_state is a filesystem bit check as opposed to inode
> and can be performed before we begin with write checks. This eliminates
> inode lock/unlock in case of error bit is set.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/file.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 4c40a2742aab..ca374cb5ffc9 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1981,6 +1981,15 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
>  	size_t count;
>  	loff_t oldsize;
>  
> +	/*
> +	 * If BTRFS flips readonly due to some impossible error
> +	 * (fs_info->fs_state now has BTRFS_SUPER_FLAG_ERROR),
> +	 * although we have opened a file as writable, we have
> +	 * to stop this write operation to ensure FS consistency.
> +	 */
> +	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> +		return -EROFS;
> +

nit: Actually can't this check be eliminated altogether or the comment
vastly simplified because BTRFS_SUPER_FLAG_ERROR check is performed only
during mount so the description in the parantheses is invalid i.e the fs
won't flip to RO because BTRFS_SUPER_FLAG_ERROR is now set in the super
block. As a matter of fact how is this flag set - because I don't see it
set in the kernel code nor in btrfs-progs ?

<snip>
Goldwyn Rodrigues Sept. 23, 2020, 2:07 p.m. UTC | #3
On 12:10 23/09, Nikolay Borisov wrote:
> 
> 
> On 21.09.20 г. 17:43 ч., Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > fs_info->fs_state is a filesystem bit check as opposed to inode
> > and can be performed before we begin with write checks. This eliminates
> > inode lock/unlock in case of error bit is set.
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >  fs/btrfs/file.c | 21 +++++++++------------
> >  1 file changed, 9 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> > index 4c40a2742aab..ca374cb5ffc9 100644
> > --- a/fs/btrfs/file.c
> > +++ b/fs/btrfs/file.c
> > @@ -1981,6 +1981,15 @@ static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
> >  	size_t count;
> >  	loff_t oldsize;
> >  
> > +	/*
> > +	 * If BTRFS flips readonly due to some impossible error
> > +	 * (fs_info->fs_state now has BTRFS_SUPER_FLAG_ERROR),
> > +	 * although we have opened a file as writable, we have
> > +	 * to stop this write operation to ensure FS consistency.
> > +	 */
> > +	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
> > +		return -EROFS;
> > +
> 
> nit: Actually can't this check be eliminated altogether or the comment
> vastly simplified because BTRFS_SUPER_FLAG_ERROR check is performed only
> during mount so the description in the parantheses is invalid i.e the fs
> won't flip to RO because BTRFS_SUPER_FLAG_ERROR is now set in the super
> block. As a matter of fact how is this flag set - because I don't see it
> set in the kernel code nor in btrfs-progs ?

You are right. This flag originated from 
acce952b0263 ("Btrfs: forced readonly mounts on errors")

However, the following commit removed writing the super in case of the
error:
68ce9682a4bb ("Btrfs: remove superblock writing after fatal error")

So, it does not land in the super flags anyways.
The flag does not make sense if we use BTRFS_FS_STATE_ERROR.

I will remove BTRFS_SUPER_FLAG_ERROR comment for now.
diff mbox series

Patch

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 4c40a2742aab..ca374cb5ffc9 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1981,6 +1981,15 @@  static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 	size_t count;
 	loff_t oldsize;
 
+	/*
+	 * If BTRFS flips readonly due to some impossible error
+	 * (fs_info->fs_state now has BTRFS_SUPER_FLAG_ERROR),
+	 * although we have opened a file as writable, we have
+	 * to stop this write operation to ensure FS consistency.
+	 */
+	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state))
+		return -EROFS;
+
 	if (!(iocb->ki_flags & IOCB_DIRECT) &&
 	    (iocb->ki_flags & IOCB_NOWAIT))
 		return -EOPNOTSUPP;
@@ -2030,18 +2039,6 @@  static ssize_t btrfs_file_write_iter(struct kiocb *iocb,
 		goto out;
 	}
 
-	/*
-	 * If BTRFS flips readonly due to some impossible error
-	 * (fs_info->fs_state now has BTRFS_SUPER_FLAG_ERROR),
-	 * although we have opened a file as writable, we have
-	 * to stop this write operation to ensure FS consistency.
-	 */
-	if (test_bit(BTRFS_FS_STATE_ERROR, &fs_info->fs_state)) {
-		inode_unlock(inode);
-		err = -EROFS;
-		goto out;
-	}
-
 	/*
 	 * We reserve space for updating the inode when we reserve space for the
 	 * extent we are going to write, so we will enospc out there.  We don't