* [PATCH 0/3] writeback: minor tweaks
@ 2016-04-12 18:43 Jens Axboe
2016-04-12 18:43 ` [PATCH 1/3] writeback: propagate the various reasons for writeback Jens Axboe
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Jens Axboe @ 2016-04-12 18:43 UTC (permalink / raw)
To: linux-block, linux-fsdevel; +Cc: jack
Hi,
Three top level patches out of the writeback throttling patchset, that
I think should make it into mainline. No functional changes in the
first two patches, and the last patch just bumps reclaim/sync
writeback to use WRITE_SYNC as a hint to the block layer.
fs/block_dev.c | 2 +-
fs/buffer.c | 2 +-
fs/f2fs/data.c | 2 +-
fs/f2fs/node.c | 2 +-
fs/fs-writeback.c | 13 +++++++++++++
fs/gfs2/meta_io.c | 3 +--
fs/mpage.c | 9 ++++-----
fs/xfs/xfs_aops.c | 2 +-
include/linux/writeback.h | 8 ++++++++
9 files changed, 31 insertions(+), 12 deletions(-)
--
Jens Axboe
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] writeback: propagate the various reasons for writeback 2016-04-12 18:43 [PATCH 0/3] writeback: minor tweaks Jens Axboe @ 2016-04-12 18:43 ` Jens Axboe 2016-04-13 9:48 ` Jan Kara 2016-04-12 18:43 ` [PATCH 2/3] writeback: add wbc_to_write() Jens Axboe ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Jens Axboe @ 2016-04-12 18:43 UTC (permalink / raw) To: linux-block, linux-fsdevel; +Cc: jack, Jens Axboe Avoid losing context by propagating the various reason why we initiate writeback. If we are doing more important reclaim or synchronous writeback, the lower levels should know about it. No intended functional changes in this patch. Signed-off-by: Jens Axboe <axboe@fb.com> --- fs/fs-writeback.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index fee81e8768c9..4300ee7b1139 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -52,6 +52,7 @@ struct wb_writeback_work { unsigned int range_cyclic:1; unsigned int for_background:1; unsigned int for_sync:1; /* sync(2) WB_SYNC_ALL writeback */ + unsigned int for_reclaim:1; /* for mem reclaim */ unsigned int auto_free:1; /* free on completion */ enum wb_reason reason; /* why was writeback initiated? */ @@ -944,6 +945,17 @@ void wb_start_writeback(struct bdi_writeback *wb, long nr_pages, work->reason = reason; work->auto_free = 1; + switch (reason) { + case WB_REASON_TRY_TO_FREE_PAGES: + case WB_REASON_FREE_MORE_MEM: + work->for_reclaim = 1; + case WB_REASON_SYNC: + work->for_sync = 1; + break; + default: + break; + } + wb_queue_work(wb, work); } @@ -1446,6 +1458,7 @@ static long writeback_sb_inodes(struct super_block *sb, .for_kupdate = work->for_kupdate, .for_background = work->for_background, .for_sync = work->for_sync, + .for_reclaim = work->for_reclaim, .range_cyclic = work->range_cyclic, .range_start = 0, .range_end = LLONG_MAX, -- 2.8.0.rc4.6.g7e4ba36 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] writeback: propagate the various reasons for writeback 2016-04-12 18:43 ` [PATCH 1/3] writeback: propagate the various reasons for writeback Jens Axboe @ 2016-04-13 9:48 ` Jan Kara 2016-04-14 15:43 ` Jens Axboe 0 siblings, 1 reply; 14+ messages in thread From: Jan Kara @ 2016-04-13 9:48 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, linux-fsdevel, jack On Tue 12-04-16 12:43:51, Jens Axboe wrote: > Avoid losing context by propagating the various reason why we > initiate writeback. If we are doing more important reclaim or > synchronous writeback, the lower levels should know about it. > > No intended functional changes in this patch. So far 'reason' is only used for tracing and I'd strongly prefer to keep it as such - otherwise the mix of flags like for_sync, for_backround, ... and 'reason' gets really messy. If you need more information propagated via wb_start_writeback() just add more flag arguments (currently there is only range_cyclic flag). Since there would be already three flag arguments, maybe it would warrant using a 'flags' argument which would be a standard bitmask of desired flags... Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] writeback: propagate the various reasons for writeback 2016-04-13 9:48 ` Jan Kara @ 2016-04-14 15:43 ` Jens Axboe 2016-04-17 12:01 ` Jan Kara 0 siblings, 1 reply; 14+ messages in thread From: Jens Axboe @ 2016-04-14 15:43 UTC (permalink / raw) To: Jan Kara; +Cc: linux-block, linux-fsdevel On 04/13/2016 03:48 AM, Jan Kara wrote: > On Tue 12-04-16 12:43:51, Jens Axboe wrote: >> Avoid losing context by propagating the various reason why we >> initiate writeback. If we are doing more important reclaim or >> synchronous writeback, the lower levels should know about it. >> >> No intended functional changes in this patch. > > So far 'reason' is only used for tracing and I'd strongly prefer to keep it > as such - otherwise the mix of flags like for_sync, for_backround, ... and > 'reason' gets really messy. If you need more information propagated via > wb_start_writeback() just add more flag arguments (currently there is only > range_cyclic flag). Since there would be already three flag arguments, > maybe it would warrant using a 'flags' argument which would be a standard > bitmask of desired flags... It'd be nicer if the tracing just ran off the functional parts, instead of having a separate argument just for tracing. It's both confusing and fragile to have two separate sets of information in there, and it's harder to keep in sync. But that's probably better left for another cleanup series. I'll flag this separately. -- Jens Axboe ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] writeback: propagate the various reasons for writeback 2016-04-14 15:43 ` Jens Axboe @ 2016-04-17 12:01 ` Jan Kara 0 siblings, 0 replies; 14+ messages in thread From: Jan Kara @ 2016-04-17 12:01 UTC (permalink / raw) To: Jens Axboe; +Cc: Jan Kara, linux-block, linux-fsdevel On Thu 14-04-16 09:43:05, Jens Axboe wrote: > On 04/13/2016 03:48 AM, Jan Kara wrote: > >On Tue 12-04-16 12:43:51, Jens Axboe wrote: > >>Avoid losing context by propagating the various reason why we > >>initiate writeback. If we are doing more important reclaim or > >>synchronous writeback, the lower levels should know about it. > >> > >>No intended functional changes in this patch. > > > >So far 'reason' is only used for tracing and I'd strongly prefer to keep it > >as such - otherwise the mix of flags like for_sync, for_backround, ... and > >'reason' gets really messy. If you need more information propagated via > >wb_start_writeback() just add more flag arguments (currently there is only > >range_cyclic flag). Since there would be already three flag arguments, > >maybe it would warrant using a 'flags' argument which would be a standard > >bitmask of desired flags... > > It'd be nicer if the tracing just ran off the functional parts, instead of > having a separate argument just for tracing. It's both confusing and fragile > to have two separate sets of information in there, and it's harder to keep > in sync. > > But that's probably better left for another cleanup series. I'll flag this > separately. Well, the point of 'reason' was that there are different call sites that issue the same type of writeback and we wanted to distinguish between them to get more insight into what is happening from the tracing. If we keep 'reason' only to document 'call site', then it is not fragile. I agree with the 'confusing' part though since you are not the first one who has tried to use the 'reason' argument for functional decision :-). Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] writeback: add wbc_to_write() 2016-04-12 18:43 [PATCH 0/3] writeback: minor tweaks Jens Axboe 2016-04-12 18:43 ` [PATCH 1/3] writeback: propagate the various reasons for writeback Jens Axboe @ 2016-04-12 18:43 ` Jens Axboe 2016-04-13 9:50 ` Jan Kara 2016-04-12 18:43 ` [PATCH 3/3] writeback: use WRITE_SYNC for reclaim or sync writeback Jens Axboe 2016-04-13 0:08 ` [PATCH 0/3] writeback: minor tweaks Dave Chinner 3 siblings, 1 reply; 14+ messages in thread From: Jens Axboe @ 2016-04-12 18:43 UTC (permalink / raw) To: linux-block, linux-fsdevel; +Cc: jack, Jens Axboe Add wbc_to_write(), which returns the write type to use, based on a struct writeback_control. No functional changes in this patch, but it prepares us for factoring other wbc fields for write type. No intended functional changes in this patch. Signed-off-by: Jens Axboe <axboe@fb.com> --- fs/block_dev.c | 2 +- fs/buffer.c | 2 +- fs/f2fs/data.c | 2 +- fs/f2fs/node.c | 2 +- fs/gfs2/meta_io.c | 3 +-- fs/mpage.c | 9 ++++----- fs/xfs/xfs_aops.c | 2 +- include/linux/writeback.h | 8 ++++++++ 8 files changed, 18 insertions(+), 12 deletions(-) diff --git a/fs/block_dev.c b/fs/block_dev.c index 3172c4e2f502..b11d4e08b9a7 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -432,7 +432,7 @@ int bdev_write_page(struct block_device *bdev, sector_t sector, struct page *page, struct writeback_control *wbc) { int result; - int rw = (wbc->sync_mode == WB_SYNC_ALL) ? WRITE_SYNC : WRITE; + int rw = wbc_to_write(wbc); const struct block_device_operations *ops = bdev->bd_disk->fops; if (!ops->rw_page || bdev_get_integrity(bdev)) diff --git a/fs/buffer.c b/fs/buffer.c index 33be29675358..28273caaf2b1 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -1697,7 +1697,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page, struct buffer_head *bh, *head; unsigned int blocksize, bbits; int nr_underway = 0; - int write_op = (wbc->sync_mode == WB_SYNC_ALL ? WRITE_SYNC : WRITE); + int write_op = wbc_to_write(wbc); head = create_page_buffers(page, inode, (1 << BH_Dirty)|(1 << BH_Uptodate)); diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index e5c762b37239..dca5d43c67a3 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -1143,7 +1143,7 @@ static int f2fs_write_data_page(struct page *page, struct f2fs_io_info fio = { .sbi = sbi, .type = DATA, - .rw = (wbc->sync_mode == WB_SYNC_ALL) ? WRITE_SYNC : WRITE, + .rw = wbc_to_write(wbc), .page = page, .encrypted_page = NULL, }; diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 118321bd1a7f..db9201f45bf1 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1397,7 +1397,7 @@ static int f2fs_write_node_page(struct page *page, struct f2fs_io_info fio = { .sbi = sbi, .type = NODE, - .rw = (wbc->sync_mode == WB_SYNC_ALL) ? WRITE_SYNC : WRITE, + .rw = wbc_to_write(wbc), .page = page, .encrypted_page = NULL, }; diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c index e137d96f1b17..ede87306caa5 100644 --- a/fs/gfs2/meta_io.c +++ b/fs/gfs2/meta_io.c @@ -37,8 +37,7 @@ static int gfs2_aspace_writepage(struct page *page, struct writeback_control *wb { struct buffer_head *bh, *head; int nr_underway = 0; - int write_op = REQ_META | REQ_PRIO | - (wbc->sync_mode == WB_SYNC_ALL ? WRITE_SYNC : WRITE); + int write_op = REQ_META | REQ_PRIO | wbc_to_write(wbc); BUG_ON(!PageLocked(page)); BUG_ON(!page_has_buffers(page)); diff --git a/fs/mpage.c b/fs/mpage.c index 6bd9fd90964e..9986c752f7bb 100644 --- a/fs/mpage.c +++ b/fs/mpage.c @@ -486,7 +486,6 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc, struct buffer_head map_bh; loff_t i_size = i_size_read(inode); int ret = 0; - int wr = (wbc->sync_mode == WB_SYNC_ALL ? WRITE_SYNC : WRITE); if (page_has_buffers(page)) { struct buffer_head *head = page_buffers(page); @@ -595,7 +594,7 @@ page_is_mapped: * This page will go to BIO. Do we need to send this BIO off first? */ if (bio && mpd->last_block_in_bio != blocks[0] - 1) - bio = mpage_bio_submit(wr, bio); + bio = mpage_bio_submit(wbc_to_write(wbc), bio); alloc_new: if (bio == NULL) { @@ -622,7 +621,7 @@ alloc_new: wbc_account_io(wbc, page, PAGE_SIZE); length = first_unmapped << blkbits; if (bio_add_page(bio, page, length, 0) < length) { - bio = mpage_bio_submit(wr, bio); + bio = mpage_bio_submit(wbc_to_write(wbc), bio); goto alloc_new; } @@ -632,7 +631,7 @@ alloc_new: set_page_writeback(page); unlock_page(page); if (boundary || (first_unmapped != blocks_per_page)) { - bio = mpage_bio_submit(wr, bio); + bio = mpage_bio_submit(wbc_to_write(wbc), bio); if (boundary_block) { write_boundary_block(boundary_bdev, boundary_block, 1 << blkbits); @@ -644,7 +643,7 @@ alloc_new: confused: if (bio) - bio = mpage_bio_submit(wr, bio); + bio = mpage_bio_submit(wbc_to_write(wbc), bio); if (mpd->use_writepage) { ret = mapping->a_ops->writepage(page, wbc); diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c index d445a64b979e..239a612ea1d6 100644 --- a/fs/xfs/xfs_aops.c +++ b/fs/xfs/xfs_aops.c @@ -393,7 +393,7 @@ xfs_submit_ioend_bio( atomic_inc(&ioend->io_remaining); bio->bi_private = ioend; bio->bi_end_io = xfs_end_bio; - submit_bio(wbc->sync_mode == WB_SYNC_ALL ? WRITE_SYNC : WRITE, bio); + submit_bio(wbc_to_write(wbc), bio); } STATIC struct bio * diff --git a/include/linux/writeback.h b/include/linux/writeback.h index d0b5ca5d4e08..719c255e105a 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -100,6 +100,14 @@ struct writeback_control { #endif }; +static inline int wbc_to_write(struct writeback_control *wbc) +{ + if (wbc->sync_mode == WB_SYNC_ALL) + return WRITE_SYNC; + + return WRITE; +} + /* * A wb_domain represents a domain that wb's (bdi_writeback's) belong to * and are measured against each other in. There always is one global -- 2.8.0.rc4.6.g7e4ba36 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] writeback: add wbc_to_write() 2016-04-12 18:43 ` [PATCH 2/3] writeback: add wbc_to_write() Jens Axboe @ 2016-04-13 9:50 ` Jan Kara 2016-04-14 15:41 ` Jens Axboe 0 siblings, 1 reply; 14+ messages in thread From: Jan Kara @ 2016-04-13 9:50 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, linux-fsdevel, jack On Tue 12-04-16 12:43:52, Jens Axboe wrote: > Add wbc_to_write(), which returns the write type to use, based on a > struct writeback_control. No functional changes in this patch, but it > prepares us for factoring other wbc fields for write type. > > No intended functional changes in this patch. I like the patch except for the function name ;). Maybe wbc_to_write_cmd() or something like that? Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] writeback: add wbc_to_write() 2016-04-13 9:50 ` Jan Kara @ 2016-04-14 15:41 ` Jens Axboe 0 siblings, 0 replies; 14+ messages in thread From: Jens Axboe @ 2016-04-14 15:41 UTC (permalink / raw) To: Jan Kara; +Cc: linux-block, linux-fsdevel On 04/13/2016 03:50 AM, Jan Kara wrote: > On Tue 12-04-16 12:43:52, Jens Axboe wrote: >> Add wbc_to_write(), which returns the write type to use, based on a >> struct writeback_control. No functional changes in this patch, but it >> prepares us for factoring other wbc fields for write type. >> >> No intended functional changes in this patch. > > I like the patch except for the function name ;). Maybe wbc_to_write_cmd() > or something like that? Yep, I like that, I've made that change. Thanks! -- Jens Axboe ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] writeback: use WRITE_SYNC for reclaim or sync writeback 2016-04-12 18:43 [PATCH 0/3] writeback: minor tweaks Jens Axboe 2016-04-12 18:43 ` [PATCH 1/3] writeback: propagate the various reasons for writeback Jens Axboe 2016-04-12 18:43 ` [PATCH 2/3] writeback: add wbc_to_write() Jens Axboe @ 2016-04-12 18:43 ` Jens Axboe 2016-04-13 0:08 ` [PATCH 0/3] writeback: minor tweaks Dave Chinner 3 siblings, 0 replies; 14+ messages in thread From: Jens Axboe @ 2016-04-12 18:43 UTC (permalink / raw) To: linux-block, linux-fsdevel; +Cc: jack, Jens Axboe If we're doing reclaim or sync IO, use WRITE_SYNC to inform the lower levels of the importance of this IO. Signed-off-by: Jens Axboe <axboe@fb.com> --- include/linux/writeback.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 719c255e105a..b2c75b8901da 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -102,7 +102,7 @@ struct writeback_control { static inline int wbc_to_write(struct writeback_control *wbc) { - if (wbc->sync_mode == WB_SYNC_ALL) + if (wbc->sync_mode == WB_SYNC_ALL || wbc->for_reclaim || wbc->for_sync) return WRITE_SYNC; return WRITE; -- 2.8.0.rc4.6.g7e4ba36 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] writeback: minor tweaks 2016-04-12 18:43 [PATCH 0/3] writeback: minor tweaks Jens Axboe ` (2 preceding siblings ...) 2016-04-12 18:43 ` [PATCH 3/3] writeback: use WRITE_SYNC for reclaim or sync writeback Jens Axboe @ 2016-04-13 0:08 ` Dave Chinner 2016-04-13 1:49 ` Jens Axboe 3 siblings, 1 reply; 14+ messages in thread From: Dave Chinner @ 2016-04-13 0:08 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, linux-fsdevel, jack On Tue, Apr 12, 2016 at 12:43:50PM -0600, Jens Axboe wrote: > Hi, > > Three top level patches out of the writeback throttling patchset, that > I think should make it into mainline. No functional changes in the > first two patches, and the last patch just bumps reclaim/sync > writeback to use WRITE_SYNC as a hint to the block layer. Whatever happened to adding a new flag to indicate that write requests can be throttled, rather than overloading WRITE_SYNC with yet another (conflicting) meaning? I've already pointed out the problems associated with tagging async, bulk writes as synchronous writes so that a lower layer can avoid throttling them. Please add a new flag for communicating whether writes can be throttled to the block layer instead of reusing WRITE_SYNC. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] writeback: minor tweaks 2016-04-13 0:08 ` [PATCH 0/3] writeback: minor tweaks Dave Chinner @ 2016-04-13 1:49 ` Jens Axboe 2016-04-13 6:09 ` Dave Chinner 2016-04-13 10:07 ` Jan Kara 0 siblings, 2 replies; 14+ messages in thread From: Jens Axboe @ 2016-04-13 1:49 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-block, linux-fsdevel, jack On 04/12/2016 06:08 PM, Dave Chinner wrote: > On Tue, Apr 12, 2016 at 12:43:50PM -0600, Jens Axboe wrote: >> Hi, >> >> Three top level patches out of the writeback throttling patchset, that >> I think should make it into mainline. No functional changes in the >> first two patches, and the last patch just bumps reclaim/sync >> writeback to use WRITE_SYNC as a hint to the block layer. > > Whatever happened to adding a new flag to indicate that write > requests can be throttled, rather than overloading WRITE_SYNC with > yet another (conflicting) meaning? WRITE_SYNC means someone will be waiting for it. This just extends it to cover more instances where that is the case (reclaim, for_sync). For background writes, we'll want to split WRITE into WRITE and WRITE_BACKGROUND. Alternatively, I can stop extending WRITE_SYNC for the other parts, and only do the background part. But I think those parts makes sense. > I've already pointed out the problems associated with tagging async, > bulk writes as synchronous writes so that a lower layer can avoid I think that's our miscommunication. It's not my intent to tag async writes as sync. If we're doing reclaim, presumable someone is waiting for those pages to be cleaned. Hence sync. Ditto for for_sync. > throttling them. Please add a new flag for communicating whether > writes can be throttled to the block layer instead of reusing > WRITE_SYNC. If someone is waiting for them, they won't be throttled as hard (WRITE_SYNC, WRITE_ODIRECT, etc). If someone is not waiting for them (WRITE), then we can throttle a bit more. That's my intent. We can add WRITE_BACKGROUND, but then we have one more write type. Why not just leave WRITE as the background type of write? -- Jens Axboe ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] writeback: minor tweaks 2016-04-13 1:49 ` Jens Axboe @ 2016-04-13 6:09 ` Dave Chinner 2016-04-13 10:07 ` Jan Kara 1 sibling, 0 replies; 14+ messages in thread From: Dave Chinner @ 2016-04-13 6:09 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block, linux-fsdevel, jack On Tue, Apr 12, 2016 at 07:49:45PM -0600, Jens Axboe wrote: > On 04/12/2016 06:08 PM, Dave Chinner wrote: > >On Tue, Apr 12, 2016 at 12:43:50PM -0600, Jens Axboe wrote: > >>Hi, > >> > >>Three top level patches out of the writeback throttling patchset, that > >>I think should make it into mainline. No functional changes in the > >>first two patches, and the last patch just bumps reclaim/sync > >>writeback to use WRITE_SYNC as a hint to the block layer. > > > >Whatever happened to adding a new flag to indicate that write > >requests can be throttled, rather than overloading WRITE_SYNC with > >yet another (conflicting) meaning? > > WRITE_SYNC means someone will be waiting for it. This just extends > it to cover more instances where that is the case (reclaim, > for_sync). > > For background writes, we'll want to split WRITE into WRITE and > WRITE_BACKGROUND. > > Alternatively, I can stop extending WRITE_SYNC for the other parts, > and only do the background part. But I think those parts makes > sense. > > >I've already pointed out the problems associated with tagging async, > >bulk writes as synchronous writes so that a lower layer can avoid > > I think that's our miscommunication. It's not my intent to tag async > writes as sync. If we're doing reclaim, presumable someone is > waiting for those pages to be cleaned. Hence sync. Ditto for > for_sync. No, it doesn't mean someone is waiting for them. It just means that reclaim or sync is in progress somewhere and that has optimistically scheduled background writeback to start (i.e. it only has effect if the flusher thread is currently idle) to get some work done before the sync on that superblock starts... And, IIRC, background writeback is aborted when new work is queued on the bdi flusher workqueue (i.e. when the real sync work is scheduled on a superblock), so it really doesn't help us to have background writeback block on locks or behave in any way like the real sync work does, because that will just delay the real timestamp controlled sync work that we need to do. > >throttling them. Please add a new flag for communicating whether > >writes can be throttled to the block layer instead of reusing > >WRITE_SYNC. > > If someone is waiting for them, they won't be throttled as hard > (WRITE_SYNC, WRITE_ODIRECT, etc). If someone is not waiting for them > (WRITE), then we can throttle a bit more. > > That's my intent. We can add WRITE_BACKGROUND, but then we have one > more write type. Why not just leave WRITE as the background type of > write? WRITE does not imply WRITE_BACKGROUND. Think about it. THere's a big difference between a AIO+DIO write, an O_SYNC buffered write and background writeback. The first should be WRITE (async write), the second WRITE_SYNC (because of the wait), and the later WRITE_BACKGROUND (can be throttled). Similarly, we do not want a set of flags that conflate content type (e.g. REQ_META) with behavioural characteristics (e.g. throttle avoidance). As per above, we should be able to specify REQ_META | WRITE, REQ_META | WRITE_SYNC and REQ_META | WRITE_BACKGROUND to indicate the three different behavioural cases of "write quickly", "write quickly, someone waiting" and "background metadata writeback in your own time". And, of course, this makes the code self documenting. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] writeback: minor tweaks 2016-04-13 1:49 ` Jens Axboe 2016-04-13 6:09 ` Dave Chinner @ 2016-04-13 10:07 ` Jan Kara 2016-04-14 15:41 ` Jens Axboe 1 sibling, 1 reply; 14+ messages in thread From: Jan Kara @ 2016-04-13 10:07 UTC (permalink / raw) To: Jens Axboe; +Cc: Dave Chinner, linux-block, linux-fsdevel, jack On Tue 12-04-16 19:49:45, Jens Axboe wrote: > On 04/12/2016 06:08 PM, Dave Chinner wrote: > >On Tue, Apr 12, 2016 at 12:43:50PM -0600, Jens Axboe wrote: > >>Hi, > >> > >>Three top level patches out of the writeback throttling patchset, that > >>I think should make it into mainline. No functional changes in the > >>first two patches, and the last patch just bumps reclaim/sync > >>writeback to use WRITE_SYNC as a hint to the block layer. > > > >Whatever happened to adding a new flag to indicate that write > >requests can be throttled, rather than overloading WRITE_SYNC with > >yet another (conflicting) meaning? > > WRITE_SYNC means someone will be waiting for it. This just extends it to > cover more instances where that is the case (reclaim, for_sync). Well, the question really is how soon waiting for the write is warranting WRITE_SYNC? Someone waits (or will likely soon wait) for background writeback if we are close to dirty limit after all. Do we want WRITE_SYNC set also in that case? In your patch 3, you change the logic to issue WRITE_SYNC writes when for_reclaim or for_sync are set and these are IMHO questionable as well - for_reclaim just means that there are dirty pages near the end of LRU and thus MM blindly issues writeback requests and hopes things will improve. for_sync means we are doing the first pass of writeback for sync(2). In either of these cases I'm not sure prioritizing writes is a clear win for the system overall, although in both cases it makes some sense. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/3] writeback: minor tweaks 2016-04-13 10:07 ` Jan Kara @ 2016-04-14 15:41 ` Jens Axboe 0 siblings, 0 replies; 14+ messages in thread From: Jens Axboe @ 2016-04-14 15:41 UTC (permalink / raw) To: Jan Kara; +Cc: Dave Chinner, linux-block, linux-fsdevel On 04/13/2016 04:07 AM, Jan Kara wrote: > On Tue 12-04-16 19:49:45, Jens Axboe wrote: >> On 04/12/2016 06:08 PM, Dave Chinner wrote: >>> On Tue, Apr 12, 2016 at 12:43:50PM -0600, Jens Axboe wrote: >>>> Hi, >>>> >>>> Three top level patches out of the writeback throttling patchset, that >>>> I think should make it into mainline. No functional changes in the >>>> first two patches, and the last patch just bumps reclaim/sync >>>> writeback to use WRITE_SYNC as a hint to the block layer. >>> >>> Whatever happened to adding a new flag to indicate that write >>> requests can be throttled, rather than overloading WRITE_SYNC with >>> yet another (conflicting) meaning? >> >> WRITE_SYNC means someone will be waiting for it. This just extends it to >> cover more instances where that is the case (reclaim, for_sync). > > Well, the question really is how soon waiting for the write is warranting > WRITE_SYNC? Someone waits (or will likely soon wait) for background > writeback if we are close to dirty limit after all. Do we want WRITE_SYNC > set also in that case? In your patch 3, you change the logic to issue > WRITE_SYNC writes when for_reclaim or for_sync are set and these are IMHO > questionable as well - for_reclaim just means that there are dirty pages > near the end of LRU and thus MM blindly issues writeback requests and hopes > things will improve. for_sync means we are doing the first pass of > writeback for sync(2). In either of these cases I'm not sure prioritizing > writes is a clear win for the system overall, although in both cases it > makes some sense. Yeah, I'm going to revert course on that a little bit. I'm refactoring to leave the SYNC part as it is, and adding an explicit WRITE_BACKGROUND. That allows me to throttle that explicitly fairly thoroughly, and throttle normal writes a bit less (and O_SYNC/O_DIRECT/etc not at all). -- Jens Axboe ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-04-17 19:17 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-12 18:43 [PATCH 0/3] writeback: minor tweaks Jens Axboe 2016-04-12 18:43 ` [PATCH 1/3] writeback: propagate the various reasons for writeback Jens Axboe 2016-04-13 9:48 ` Jan Kara 2016-04-14 15:43 ` Jens Axboe 2016-04-17 12:01 ` Jan Kara 2016-04-12 18:43 ` [PATCH 2/3] writeback: add wbc_to_write() Jens Axboe 2016-04-13 9:50 ` Jan Kara 2016-04-14 15:41 ` Jens Axboe 2016-04-12 18:43 ` [PATCH 3/3] writeback: use WRITE_SYNC for reclaim or sync writeback Jens Axboe 2016-04-13 0:08 ` [PATCH 0/3] writeback: minor tweaks Dave Chinner 2016-04-13 1:49 ` Jens Axboe 2016-04-13 6:09 ` Dave Chinner 2016-04-13 10:07 ` Jan Kara 2016-04-14 15:41 ` Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.