[PATCH 0/3] writeback: minor tweaks

[PATCH 0/3] writeback: minor tweaks

[Jens Axboe]

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

[PATCH 1/3] writeback: propagate the various reasons for writeback

[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

[PATCH 2/3] writeback: add wbc_to_write()

[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

[PATCH 3/3] writeback: use WRITE_SYNC for reclaim or sync writeback

[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

Re: [PATCH 0/3] writeback: minor tweaks

[Dave Chinner]

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

Re: [PATCH 0/3] writeback: minor tweaks

[Jens Axboe]

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

Re: [PATCH 0/3] writeback: minor tweaks

[Dave Chinner]

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

Re: [PATCH 1/3] writeback: propagate the various reasons for writeback

[Jan Kara]

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

Re: [PATCH 2/3] writeback: add wbc_to_write()

[Jan Kara]

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

Re: [PATCH 0/3] writeback: minor tweaks

[Jan Kara]

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

Re: [PATCH 0/3] writeback: minor tweaks

[Jens Axboe]

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

Re: [PATCH 2/3] writeback: add wbc_to_write()

[Jens Axboe]

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

Re: [PATCH 1/3] writeback: propagate the various reasons for writeback

[Jens Axboe]

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

Re: [PATCH 1/3] writeback: propagate the various reasons for writeback

[Jan Kara]

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