| Message ID | 20200921144353.31319-8-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | BTRFS DIO inode locking/D_SYNC fix | expand |
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
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>
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 --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