| Message ID | 20230329001308.1275299-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] btrfs: fix fast csum detection | expand |
On Wed, Mar 29, 2023 at 09:13:06AM +0900, Christoph Hellwig wrote: > The sync_writers field communicates if an inode has outstanding > synchronous writeback. The better way is to look at the REQ_SYNC > flag in the bio, which is set by the writeback code for WB_SYNC_ALL > writeback and can communicate the need for sync writeback without > an inode field. This does not go into much detail why it is 'better'. Tracking the status from the outside as sync_writers applies to the whole inode and not just the submission context of the individual bios so there's a difference. Also the subject should say something about changing the decision logic, removal of sync_writes is just a side effect. > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/btrfs/bio.c | 7 +++---- > fs/btrfs/btrfs_inode.h | 3 --- > fs/btrfs/file.c | 9 --------- > fs/btrfs/inode.c | 1 - > fs/btrfs/transaction.c | 2 -- > 5 files changed, 3 insertions(+), 19 deletions(-) > > diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c > index cf09c6271edbee..c851a3526911f6 100644 > --- a/fs/btrfs/bio.c > +++ b/fs/btrfs/bio.c > @@ -552,11 +552,10 @@ static void run_one_async_free(struct btrfs_work *work) > static bool should_async_write(struct btrfs_bio *bbio) > { > /* > - * If the I/O is not issued by fsync and friends, (->sync_writers != 0), > - * then try to defer the submission to a workqueue to parallelize the > - * checksum calculation. > + * Try to defer the submission to a workqueue to parallelize the > + * checksum calculation unless the I/O is issued synchronously. > */ > - if (atomic_read(&bbio->inode->sync_writers)) > + if (op_is_sync(bbio->bio.bi_opf)) So if there's sync_writers set from any of - btrfs_do_write_iter - start_ordered_ops - btrfs_write_marked_extents then should_async_write would always return false. The change in behaviour looks like "write everything for this inode synchronously" vs "write some bios synchronously". I don't see the 1:1 correspondence. > return false;
diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c index cf09c6271edbee..c851a3526911f6 100644 --- a/fs/btrfs/bio.c +++ b/fs/btrfs/bio.c @@ -552,11 +552,10 @@ static void run_one_async_free(struct btrfs_work *work) static bool should_async_write(struct btrfs_bio *bbio) { /* - * If the I/O is not issued by fsync and friends, (->sync_writers != 0), - * then try to defer the submission to a workqueue to parallelize the - * checksum calculation. + * Try to defer the submission to a workqueue to parallelize the + * checksum calculation unless the I/O is issued synchronously. */ - if (atomic_read(&bbio->inode->sync_writers)) + if (op_is_sync(bbio->bio.bi_opf)) return false; /* diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index 9dc21622806ef4..8666ace8879302 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -116,9 +116,6 @@ struct btrfs_inode { unsigned long runtime_flags; - /* Keep track of who's O_SYNC/fsyncing currently */ - atomic_t sync_writers; - /* full 64 bit generation number, struct vfs_inode doesn't have a big * enough field for this. */ diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 5cc5a1faaef5b5..4b9433b96f4b8e 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1651,7 +1651,6 @@ ssize_t btrfs_do_write_iter(struct kiocb *iocb, struct iov_iter *from, struct file *file = iocb->ki_filp; struct btrfs_inode *inode = BTRFS_I(file_inode(file)); ssize_t num_written, num_sync; - const bool sync = iocb_is_dsync(iocb); /* * If the fs flips readonly due to some impossible error, although we @@ -1664,9 +1663,6 @@ ssize_t btrfs_do_write_iter(struct kiocb *iocb, struct iov_iter *from, if (encoded && (iocb->ki_flags & IOCB_NOWAIT)) return -EOPNOTSUPP; - if (sync) - atomic_inc(&inode->sync_writers); - if (encoded) { num_written = btrfs_encoded_write(iocb, from, encoded); num_sync = encoded->len; @@ -1686,9 +1682,6 @@ ssize_t btrfs_do_write_iter(struct kiocb *iocb, struct iov_iter *from, num_written = num_sync; } - if (sync) - atomic_dec(&inode->sync_writers); - current->backing_dev_info = NULL; return num_written; } @@ -1733,9 +1726,7 @@ static int start_ordered_ops(struct inode *inode, loff_t start, loff_t end) * several segments of stripe length (currently 64K). */ blk_start_plug(&plug); - atomic_inc(&BTRFS_I(inode)->sync_writers); ret = btrfs_fdatawrite_range(inode, start, end); - atomic_dec(&BTRFS_I(inode)->sync_writers); blk_finish_plug(&plug); return ret; diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 865d56ff2ce150..b505b205c1b142 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -8479,7 +8479,6 @@ struct inode *btrfs_alloc_inode(struct super_block *sb) ei->io_tree.inode = ei; extent_io_tree_init(fs_info, &ei->file_extent_tree, IO_TREE_INODE_FILE_EXTENT); - atomic_set(&ei->sync_writers, 0); mutex_init(&ei->log_mutex); btrfs_ordered_inode_tree_init(&ei->ordered_tree); INIT_LIST_HEAD(&ei->delalloc_inodes); diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index c8e503e5db4cdb..6587a13de0aedb 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1055,7 +1055,6 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, u64 start = 0; u64 end; - atomic_inc(&BTRFS_I(fs_info->btree_inode)->sync_writers); while (!find_first_extent_bit(dirty_pages, start, &start, &end, mark, &cached_state)) { bool wait_writeback = false; @@ -1091,7 +1090,6 @@ int btrfs_write_marked_extents(struct btrfs_fs_info *fs_info, cond_resched(); start = end + 1; } - atomic_dec(&BTRFS_I(fs_info->btree_inode)->sync_writers); return werr; }
The sync_writers field communicates if an inode has outstanding synchronous writeback. The better way is to look at the REQ_SYNC flag in the bio, which is set by the writeback code for WB_SYNC_ALL writeback and can communicate the need for sync writeback without an inode field. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/btrfs/bio.c | 7 +++---- fs/btrfs/btrfs_inode.h | 3 --- fs/btrfs/file.c | 9 --------- fs/btrfs/inode.c | 1 - fs/btrfs/transaction.c | 2 -- 5 files changed, 3 insertions(+), 19 deletions(-)