diff mbox

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

Message ID 1460486633-26099-2-git-send-email-axboe@fb.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jens Axboe April 12, 2016, 6:43 p.m. UTC
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(+)

Comments

Jan Kara April 13, 2016, 9:48 a.m. UTC | #1
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
Jens Axboe April 14, 2016, 3:43 p.m. UTC | #2
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.
Jan Kara April 17, 2016, 12:01 p.m. UTC | #3
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
diff mbox

Patch

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,