Add a super operation for writeback

---
From: Daniel Phillips <[email protected]>
Subject: [PATCH] Add a super operation for writeback

Add a "writeback" super operation to be called in the form:

        progress = sb->s_op->writeback(sb, &wbc, &pages);

The filesystem is expected to flush some inodes to disk and return progress of at least 1, or if no inodes are flushed, return progress of zero. The filesystem should try to flush at least the number of pages specified in *pages, or if that is not possible, return approximately the number of pages not flushed into *pages.

Within the ->writeback callback, the filesystem should call inode_writeback_done(inode) for each inode flushed (and therefore set clean) or inode_writeback_touch(inode) for any inode that will be retained dirty in cache.

Signed-off-by Daniel Phillips <[email protected]>

Signed-off-by OGAWA Hirofumi <[email protected]>

---

 fs/fs-writeback.c  |   59 +++++++++++++++++++++++++++++++++++++++++++++++++---
 include/linux/fs.h |    4 +++
 2 files changed, 60 insertions(+), 3 deletions(-)

diff -puN fs/fs-writeback.c~core-writeback fs/fs-writeback.c

fs/fs-writeback.c renamed from fs/fs-writeback.c~core-writeback +17 -0

@@ -192,6 +192,35 @@ void inode_wb_list_del(struct inode *ino
 }
 
 /*
+ * Remove inode from writeback list if clean.
+ */
+void inode_writeback_done(struct inode *inode)
+{
+	struct backing_dev_info *bdi = inode_to_bdi(inode);
+
+	spin_lock(&bdi->wb.list_lock);
+	spin_lock(&inode->i_lock);
+	if (!(inode->i_state & I_DIRTY))
+		list_del_init(&inode->i_wb_list);
+	spin_unlock(&inode->i_lock);
+	spin_unlock(&bdi->wb.list_lock);
+}
+EXPORT_SYMBOL_GPL(inode_writeback_done);
+
+/*
+ * Add inode to writeback dirty list with current time.

fs/fs-writeback.c renamed from fs/fs-writeback.c~core-writeback +18 -3

+
+/*
  * Redirty an inode: set its when-it-was dirtied timestamp and move it to the
  * furthest end of its superblock's dirty-inode list.
  *

@@ -593,9 +622,9 @@ static long writeback_chunk_size(struct
  *
  * Return the number of pages and/or inodes written.
  */
-static long writeback_sb_inodes(struct super_block *sb,
-				struct bdi_writeback *wb,
-				struct wb_writeback_work *work)
+static long __writeback_sb_inodes(struct super_block *sb,
+				  struct bdi_writeback *wb,
+				  struct wb_writeback_work *work)
 {
 	struct writeback_control wbc = {
 		.sync_mode		= work->sync_mode,

@@ -710,6 +739,30 @@ static long writeback_sb_inodes(struct s
 	return wrote;
 }
 
+static long writeback_sb_inodes(struct super_block *sb,
+				struct bdi_writeback *wb,
+				struct wb_writeback_work *work)
+{
+	if (sb->s_op->writeback) {
+		struct writeback_control wbc = {
+			.sync_mode		= work->sync_mode,
+			.tagged_writepages	= work->tagged_writepages,
+			.for_kupdate		= work->for_kupdate,
+			.for_background		= work->for_background,
+			.for_sync		= work->for_sync,
+			.range_cyclic		= work->range_cyclic,
+		};

Instead of re-implementing part of fs/fs-writeback.c, use a proposed net ->writeback super operation to drive delta writeback. For each inode that is cleaned, call inode_writeback_done(inode). For each inode that will be kept dirty in cache, call inode_writeback_touch so that the inode appears young to fs-writeback and does not trigger repeated ->writeback flushes.

Signed-off-by Daniel Phillips <[email protected]>

Hi,

This is the first of four core changes we would like for Tux3. We start with a hard one and suggest a simple solution.

The first patch in this series adds a new super operation to write back multiple inodes in a single call. The second patch applies to our linux-tux3 repository at git.kernel.org to demonstrate how this interface is used, and removes about 450 lines of workaround code.

Traditionally, core kernel tells each mounted filesystems which dirty pages of which inodes should be flushed to disk, but unfortunately, is blissfully ignorant of filesystem-specific ordering constraints. This scheme was really invented for Ext2 and has not evolved much recently. Tux3, with its strong ordering and optimized delta commit, cannot tolerate random flushing and therefore takes responsibility for flush ordering itself. On the other hand, Tux3 has no good way to know when is the right time to flush, but core is good at that. This proposed API harmonizes those two competencies so that Tux3 and core each take care of what they are good at, and not more.

The API extension consists of a new writeback hook and two helpers to be uses within the hook. The hook sits about halfway down the fs-writeback stack, just after core has determined that some dirty inodes should be flushed to disk and just before it starts thinking about which inodes those should be. At that point, core calls Tux3 instead of continuing on down the usual do_writepages path. Tux3 responds by staging a new delta commit, using the new helpers to tell core which inodes were flushed versus being left dirty in cache. This is pretty much the same behavior as the traditional writeout path, but less convoluted, probably more efficient, and certainly easier to analyze.

The new writeback hook looks like:

     progress = sb->s_op->writeback(sb,&writeback_control,&nr_pages);

This should be self-explanatory: nr_pages and progress have the semantics of existing usage in fs-writeback; writeback_control is ignored by Tux3, but that is only because Tux3 always flushes everything and does not require hints for now. We can safely assume that&wbc or equivalent is wanted here. An obvious wart is the overlap between "progress" and "nr_pages", but fs-writeback thinks that way, so it would not make much sense to improve one without improving the other.

Tux3 necessarily keeps its own dirty inode list, which is an area of overlap with fs-writeback. In a perfect world, there would be just one dirty inode list per superblock, on which both fs-writeback and Tux3 would operate. That would be a deeper core change than seems appropriate right now.

Potential races are a big issue with this API, which is no surprise. The fs-writeback scheme requires keeping several kinds of object in
sync: tux3 dirty inode lists, fs-writeback dirty inode lists and inode dirty state. The new helpers inode_writeback_done(inode) and inode_writeback_touch(inode) take care of that while hiding internal details of the fs-writeback implementation.

Tux3 calls inode_writeback_done when it has flushed an inode and marked it clean, or calls inode_writeback_touch if it intends to retain a dirty inode in cache. These have simple implementations. The former just removes a clean inode from any fs-writeback list. The latter updates the inode's dirty timestamp so that fs-writeback does not keep trying flush it. Both these things could be done more efficiently by re-engineering fs-writeback, but we prefer to work with the existing scheme for now.

Hirofumi's masterful hack nicely avoided racy removal of inodes from the writeback list by taking an internal fs-writeback lock inside filesystem code. The new helper requires dropping i_lock inside filesystem code and retaking it in the helper, so inode redirty can race with writeback list removal. This does not seem to present a problem because filesystem code is able to enforce strict alternation of cleaning and calling the helper. As an offsetting advantage, writeback lock contention is reduced.

Compared to Hirofumi's hack, the cost of this interface is one additional spinlock per inode_writeback_done, which is insignificant compared to the convoluted code path that is avoided. Regards,

Daniel

fs/fs-writeback.c void inode_wb_list_del(struct inode *ino · line 221

Quoted 13 lines · On Sun, Jun 01, 2014 at 02:41:02PM -0700, I wrote:

fs/fs-writeback.c renamed from fs/fs-writeback.c~core-writeback +13 -0

+
+/*
+ * Add inode to writeback dirty list with current time.
+ */
+void inode_writeback_touch(struct inode *inode)
+{
+	struct backing_dev_info *bdi = inode->i_sb->s_bdi;
+	spin_lock(&bdi->wb.list_lock);
+	inode->dirtied_when = jiffies;
+	list_move(&inode->i_wb_list, &bdi->wb.b_dirty);
+	spin_unlock(&bdi->wb.list_lock);
+}
+EXPORT_SYMBOL_GPL(inode_writeback_touch);

You should be able to use redirty_tail() for this....

Hmmmm - this is using the wb dirty lists and locks, but you don't pass the wb structure to the writeback callout? That seem wrong to me - why would you bother manipulating these lists if you aren't using them to track dirty inodes in the first place?

From Tux3's point of view, the wb lists just allow fs-writeback to determine when to call

The first thing that __writeback_sb_inodes() does is create a struct writeback_control from the wb_writeback_work. That should be done here and passed to __writeback_sb_inodes(), which should be renamed "generic_writeback_sb_inodes()". Also, all the fields in the wbc need to be initialised correctly (i.e including range_start/end).

Further, a writeback implementation will need to use the generic bdi list and lock structures and so we need to pass the bdi_writeback. Similarly, if we are going to pass nr_pages, we might as well pass the entire work structure.

Finally, I don't like the way the wb->list_lock is treated differently by this code. I suspect that we need to rationalise the layering of the wb->list_lock as it is currently not clear what it protects and what (nested) layers of the writeback code actually require it.

What I'd like to see is this work:

struct super_ops ... = {
....
.writeback = generic_writeback_sb_inodes,
....

And that means writeback_sb_inodes() would become:

static long writeback_sb_inodes(struct super_block *sb,
struct bdi_writeback *wb, struct wb_writeback_work *work)

{
	struct writeback_control wbc = {
		.sync_mode		= work->sync_mode,
		.tagged_writepages	= work->tagged_writepages,
		.for_kupdate		= work->for_kupdate,
		.for_background		= work->for_background,
		.for_sync		= work->for_sync,
		.range_cyclic		= work->range_cyclic,
		.range_start		= 0,
		.range_end		= LLONG_MAX,
	};

	if (sb->s_op->writeback)
		return sb->s_op->writeback(sb, wb, work, &wbc);

	return generic_writeback_sb_inodes(sb, wb, work, &wbc);
}

And the higher/lower layers deal with wb->list_lock appropriately.

I get very worried whenever I see locks inside inode->i_lock. In general, i_lock is supposed to be the innermost lock that is taken, and there are very few exceptions to that - the inode LRU list is one of the few.

I don't know what the tuxnode->lock is, but I found this:

* inode->i_lock
* tuxnode->lock (to protect tuxnode data)
* tuxnode->dirty_inodes_lock (for i_ddc->dirty_inodes,
* Note: timestamp can be updated
* outside inode->i_mutex)

and this:

* inode->i_lock
* tuxnode->lock
* sb->dirty_inodes_lock

Which indicates that you take a filesystem global lock a couple of layers underneath the VFS per-inode i_lock. I'd suggest you want to separate the use of the vfs inode ilock from the locking heirarchy of the tux3 inode....

Quoted 15 lines · Dave Chinner

fs/fs-writeback.c void inode_wb_list_del(struct inode *ino · line 221

Quoted 13 lines · On Sun, Jun 01, 2014 at 02:41:02PM -0700, I wrote:

fs/fs-writeback.c renamed from fs/fs-writeback.c~core-writeback +13 -0

+
+/*
+ * Add inode to writeback dirty list with current time.
+ */
+void inode_writeback_touch(struct inode *inode)
+{
+	struct backing_dev_info *bdi = inode->i_sb->s_bdi;
+	spin_lock(&bdi->wb.list_lock);
+	inode->dirtied_when = jiffies;
+	list_move(&inode->i_wb_list, &bdi->wb.b_dirty);
+	spin_unlock(&bdi->wb.list_lock);
+}
+EXPORT_SYMBOL_GPL(inode_writeback_touch);

You should be able to use redirty_tail() for this....

Redirty_tail nearly works, but "if (!list_empty(&wb->b_dirty))" is not correct because the inode needs to end up on the dirty list whether it was already there or not.

This requirement is analogous to __mark_inode_dirty() and must tolerate similar races. At the microoptimization level, calling redirty_tail from inode_writeback_touch would be less efficient and more bulky. Another small issue is, redirty_tail does not always update the timestamp, which could trigger some bogus writeback events.

Quoted 4 lines

Hmmmm - this is using the wb dirty lists and locks, but you don't pass the wb structure to the writeback callout? That seem wrong to me - why would you bother manipulating these lists if you aren't using them to track dirty inodes in the first place?

From Tux3's point of view, the wb lists just allow fs-writeback to determine when to call ->writeback(). We agree that inode lists are a suboptimal mechanism, but that is how fs-writeback currently thinks. It would be better if our inodes never go onto wb lists in the first place, provided that fs-writeback can still drive writeback appropriately.

Perhaps fs-writeback should have an option to work without inode lists at all, and just maintain writeback scheduling statistics in the superblock or similar. That would be a big change, more on the experimental side. We would be happy to take it on after merge, hopefully for the benefit of other filesystems besides Tux3.

What we pass to ->writeback() is just a matter of taste at the moment because we currently ignore everything except &work->nr_pages. Anything sane is fine. Note that bdi_writeback is already available from bdi->wb, the "default writeback info", whatever that means. A quick tour of existing usage suggests that you can reliably obtain the wb that way, but a complete audit would be needed.

Most probably, this API will evolve as new users arrive, and also as our Tux3 usage becomes more sophisticated. For now, Tux3 just flushes everything without asking questions. Exactly how that might change in the future is hard to predict. You are in a better position to know what XFS would require in order to use this interface.

Quoted 5 lines

Finally, I don't like the way the wb->list_lock is treated differently by this code. I suspect that we need to rationalise the layering of the wb->list_lock as it is currently not clear what it protects and what (nested) layers of the writeback code actually require it.

One design goal of this proposed writeback interface is to hide the wb list lock entirely from the filesystem so core writeback can evolve more easily.

This is not cast in stone, but it smells
like decent factoring. We can save some spinlocks by violating that factoring (as Hirofumi's original hack did) at the cost of holding a potentially busy wb lock longer, which does not seem like a good trade.

When I followed the advice of Dave Chinner:
"We're not going to merge that page forking stuff (like you were told at LSF 2013 more than a year ago:
http://lwn.net/Articles/548091/) without rigorous design review and a demonstration of the solutions to all the hard corner cases it has"
given in his e-mail related with the presentation of the latest version of the Tux3 file system (see [1]) and read the linked article, I found in the second comments:
"Parts of this almost sound like it either a.) overlaps with or b.) would benefit greatly from something similar to Featherstitch [[2]]."

Could it be that we have with Featherstitch a general solution already that is said to be even "file system agnostic"? Honestly, I thought that something like this would make its way into the Linux code base.

Quoted 13 lines · Christian Stroetmann

When I followed the advice of Dave Chinner:
"We're not going to merge that page forking stuff (like you were told at LSF 2013 more than a year ago:
http://lwn.net/Articles/548091/) without rigorous design review and a demonstration of the solutions to all the hard corner cases it has"
given in his e-mail related with the presentation of the latest version of the Tux3 file system (see [1]) and read the linked article, I found in the second comments:
"Parts of this almost sound like it either a.) overlaps with or b.) would benefit greatly from something similar to Featherstitch [[2]]."

Quoted 4 lines

Could it be that we have with Featherstitch a general solution already that is said to be even "file system agnostic"? Honestly, I thought that something like this would make its way into the Linux code base.

Here's what I said about the last proposal (a few months ago) for integrating featherstitch into the kernel:

http://www.spinics.net/lists/linux-fsdevel/msg72799.html

It's not a viable solution.

Cheers,

Dave.

Quoted 4 lines · Daniel Phillips

Redirty_tail nearly works, but "if (!list_empty(&wb->b_dirty))" is not correct because the inode needs to end up on the dirty list whether it was already there or not.

redirty_tail() always moves the inode to the end of the dirty list.

Quoted 6 lines

This requirement is analogous to __mark_inode_dirty() and must tolerate similar races. At the microoptimization level, calling redirty_tail from inode_writeback_touch would be less efficient and more bulky. Another small issue is, redirty_tail does not always update the timestamp, which could trigger some bogus writeback events.

redirty_tail does not update the timestamp when it doesn't need to change. If it needs to be changed because the current value would violate the time ordering requirements of the list, it rewrites it.

So there is essentially no functional difference between the new function and redirty_tail....

Quoted 5 lines

Quoted 5 lines

Hmmmm - this is using the wb dirty lists and locks, but you don't pass the wb structure to the writeback callout? That seem wrong to me - why would you bother manipulating these lists if you aren't using them to track dirty inodes in the first place?

Quoted 6 lines

From Tux3's point of view, the wb lists just allow fs-writeback to determine when to call ->writeback(). We agree that inode lists are a suboptimal mechanism, but that is how fs-writeback currently thinks. It would be better if our inodes never go onto wb lists in the first place, provided that fs-writeback can still drive writeback appropriately.

It can't, and definitely not with the callout you added.

However, we already avoid the VFS writeback lists for certain filesystems for pure metadata. e.g. XFS does not use the VFS dirty inode lists for inode metadata changes. They get tracked internally by the transaction subsystem which does it's own writeback according to the requirements of journal space availability.

This is done simply by not calling mark_inode_dirty() on any metadata only change. If we want to do the same for data, then we'd simply not call mark_inode_dirty() in the data IO path. That requires a custom ->set_page_dirty method to be provided by the filesystem that didn't call

	__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);

and instead did it's own thing.

So the per-superblock dirty tracking is something we can do right now, and some filesystems do it for metadata. The missing piece for data is writeback infrastructure capable of deferring to superblocks for writeback rather than BDIs....

Quoted 5 lines

Perhaps fs-writeback should have an option to work without inode lists at all, and just maintain writeback scheduling statistics in the superblock or similar. That would be a big change, more on the experimental side. We would be happy to take it on after merge, hopefully for the benefit of other filesystems besides Tux3.

Well, I don't see it that way. If you have a requirement to be able to track dirty inodes internally, then lets move to that implement that infrastructure rather than hacking around with callouts that only have a limited shelf-life.

XFS already has everything it needs internally to track dirty inodes. In fact, we used to do data writeback from within XFS and we slowly removed it as the generic writeback code was improved made the XFS code redundant.

That said, parallelising writeback so we can support hundreds of GB/s of delayed allocation based writeback is something we eventually need to do, and that pretty much means we need to bring dirty data inode tracking back into XFS.

So what we really need is writeback infrastructure that is:

a) independent of the dirty inode lists
b) implements background, periodic and immediate writeback
c) can be driven by BDI or superblock

IOWs, the abstraction we need for this writeback callout is not at the writeback_sb_inodes() layer, it's above the dirty inode list queuing layer. IOWs, the callout needs to be closer to the wb_do_writeback() layer which drives all the writeback work including periodic and background writeback, and the interactions between foreground and background work that wb_writeback() uses needs to be turned into a bunch of helpers...

Quoted 5 lines

This is not cast in stone, but it smells
like decent factoring. We can save some spinlocks by violating that factoring (as Hirofumi's original hack did) at the cost of holding a potentially busy wb lock longer, which does not seem like a good trade.

If we abstract at a higher layer, the wb lock protects just the work queuing and dispatch, and everything else can be done by the superblock callout. If the superblock callout is missing, then we simply fall down to the existing wb_writeback() code that retakes the lock and does it's work....

Let's get the layering and abstraction in the right place the first time, rather having to revist this in the not-to-distant future.

From 8672ba5915e94a833321435d979615ac7059c789 Mon Sep 17 00:00:00 2001

From d030d328757b160b39b252e82811a94843513cfc Mon Sep 17 00:00:00 2001

Hi Dave,

Quoted 7 lines · Dave Chinner

Quoted 4 lines · Daniel Phillips

Redirty_tail nearly works, but "if (!list_empty(&wb->b_dirty))" is not correct because the inode needs to end up on the dirty list whether it was already there or not.

redirty_tail() always moves the inode to the end of the dirty list.

Quoted 6 lines

Quoted 6 lines

This requirement is analogous to __mark_inode_dirty() and must tolerate similar races. At the microoptimization level, calling redirty_tail from inode_writeback_touch would be less efficient and more bulky. Another small issue is, redirty_tail does not always update the timestamp, which could trigger some bogus writeback events.

Quoted 6 lines

redirty_tail does not update the timestamp when it doesn't need to change. If it needs to be changed because the current value would violate the time ordering requirements of the list, it rewrites it.

So there is essentially no functional difference between the new function and redirty_tail....

Hirofumi, would you care to comment?

Hi Dave,

Quoted 5 lines · Dave Chinner

Quoted 4 lines · Daniel Phillips

Quoted 4 lines

Hmmmm - this is using the wb dirty lists and locks, but you don't pass the wb structure to the writeback callout? That seem wrong to me - why would you bother manipulating these lists if you aren't using them to track dirty inodes in the first place?

Quoted 7 lines

Quoted 6 lines

From Tux3's point of view, the wb lists just allow fs-writeback to determine when to call ->writeback(). We agree that inode lists are a suboptimal mechanism, but that is how fs-writeback currently thinks. It would be better if our inodes never go onto wb lists in the first place, provided that fs-writeback can still drive writeback appropriately.

It can't, and definitely not with the callout you added.

Obviously, fs-writeback does not get to choose inodes or specific pages with our proposal (which is the whole point) but it still decides when to call Tux3 vs some other filesystem on the same device, and is still able to indicate how much data it thinks should be written out. Even though we ignore the latter suggestion and always flush everything, we appreciate the timing of those callbacks very much, because they give us exactly the pressure sensitive batching behavior we want.

Quoted 16 lines

However, we already avoid the VFS writeback lists for certain filesystems for pure metadata. e.g. XFS does not use the VFS dirty inode lists for inode metadata changes. They get tracked internally by the transaction subsystem which does it's own writeback according to the requirements of journal space availability.

This is done simply by not calling mark_inode_dirty() on any metadata only change. If we want to do the same for data, then we'd simply not call mark_inode_dirty() in the data IO path. That requires a custom ->set_page_dirty method to be provided by the filesystem that didn't call

	__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);

and instead did it's own thing.

Quoted 4 lines

So the per-superblock dirty tracking is something we can do right now, and some filesystems do it for metadata. The missing piece for data is writeback infrastructure capable of deferring to superblocks for writeback rather than BDIs....

We agree that fs-writeback inode lists are broken for anything more sophisticated than Ext2.

An advantage of the patch under
consideration is that it still lets fs-writeback mostly work the way it has worked for the last few years, except for not allowing it to pick specific inodes and data pages for writeout. As far as I can see, it still balances writeout between different filesystems on the same block device pretty well.

Quoted 5 lines

Quoted 5 lines

Perhaps fs-writeback should have an option to work without inode lists at all, and just maintain writeback scheduling statistics in the superblock or similar. That would be a big change, more on the experimental side. We would be happy to take it on after merge, hopefully for the benefit of other filesystems besides Tux3.

Quoted 4 lines

Well, I don't see it that way. If you have a requirement to be able to track dirty inodes internally, then lets move to that implement that infrastructure rather than hacking around with callouts that only have a limited shelf-life.

What you call shelf-life, I call iteration. Small changes are beautiful. Apologies for the rhetoric, content is below.

Quoted 16 lines

XFS already has everything it needs internally to track dirty inodes. In fact, we used to do data writeback from within XFS and we slowly removed it as the generic writeback code was improved made the XFS code redundant.

That said, parallelising writeback so we can support hundreds of GB/s of delayed allocation based writeback is something we eventually need to do, and that pretty much means we need to bring dirty data inode tracking back into XFS.

So what we really need is writeback infrastructure that is:

a) independent of the dirty inode lists
b) implements background, periodic and immediate writeback
c) can be driven by BDI or superblock

Quoted 7 lines

IOWs, the abstraction we need for this writeback callout is not at the writeback_sb_inodes() layer, it's above the dirty inode list queuing layer. IOWs, the callout needs to be closer to the wb_do_writeback() layer which drives all the writeback work including periodic and background writeback, and the interactions between foreground and background work that wb_writeback() uses needs to be turned into a bunch of helpers...

I agree, and that is what Tux3 really wants. I just wonder about the wisdom of embarking on a big change to fs-writeback when a small change will do. How will fs-writeback balancing between different filesystems on the same device work? What will those helpers look like? How long will it take to prove the new scheme stable? How can we prove that no existing fs-writeback clients will regress?

What about a sporting proposal: you post a patch that trumps ours in elegance, that you could in theory use for XFS. We verify that it works for Tux3 at least as well as the current patch. We also test it for you.

Quoted 11 lines

Quoted 5 lines

This is not cast in stone, but it smells
like decent factoring. We can save some spinlocks by violating that factoring (as Hirofumi's original hack did) at the cost of holding a potentially busy wb lock longer, which does not seem like a good trade.

If we abstract at a higher layer, the wb lock protects just the work queuing and dispatch, and everything else can be done by the superblock callout. If the superblock callout is missing, then we simply fall down to the existing wb_writeback() code that retakes the lock and does it's work....

Quoted 3 lines

Let's get the layering and abstraction in the right place the first time, rather having to revist this in the not-to-distant future.

That would be ideal, but incremental also has its attractions. In that vein, here is a rewrite along the lines you suggested yesterday.

Quoted 4 lines · Daniel Phillips

Hi Dave,

Quoted 2 lines · Dave Chinner

Quoted 1 line · Daniel Phillips

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Redirty_tail nearly works, but "if (!list_empty(&wb->b_dirty))" is not correct because the inode needs to end up on the dirty list whether it was already there or not.

redirty_tail() always moves the inode to the end of the dirty list.

It doesn't move inode to end of the dirty if wb.b_dirty is empty (I.e. it can move from wb.b_io to wb.b_dirty too).

It has difference.

Say, tail->dirtied_when == 1, inode->dirtied_when == 2, and now == 30 (tail->dirtied_when is expired at 31 with default config). In this case, redirty_tail() doesn't update ->dirtied_when.

Quoted 2 lines

It doesn't move inode to end of the dirty if wb.b_dirty is empty (I.e. it can move from wb.b_io to wb.b_dirty too).

Um, really? What code are you reading? From 3.15-rc8:

static void redirty_tail(struct inode *inode, struct bdi_writeback *wb)

{
        assert_spin_locked(&wb->list_lock);
        if (!list_empty(&wb->b_dirty)) {
                struct inode *tail;

                tail = wb_inode(wb->b_dirty.next);
                if (time_before(inode->dirtied_when, tail->dirtied_when))
                        inode->dirtied_when = jiffies;
        }
        list_move(&inode->i_wb_list, &wb->b_dirty);
}

The list_move() is not conditional at all and so the inode is *always* moved to the tail of wb->b_dirty....

Quoted 5 lines

It has difference.

Say, tail->dirtied_when == 1, inode->dirtied_when == 2, and now == 30 (tail->dirtied_when is expired at 31 with default config). In this case, redirty_tail() doesn't update ->dirtied_when.

OK, that's a different issue, and is actually handled by requeue_inode(), which is called to put inodes back on the correct dirty list when IO completes. I think that if you are going to use the wb dirty inode lists, you should probably use the existing functions to manage the inode lists appropriately rather than creating your own writeback list lifecycle.

If tux3 wants it's own dirty inode list lifecycles, then that's where avoiding the wb lists completely is an appropriate design point. I don't want to hack little bits and pieces all over the writeback code to support what tux3 is doing right now if it's going to do something else real soon. When tux3 moves to use it's own internal lists these new funcitons just have to be removed again, so let's skip the hack step we are at right now and go straight for supporting the "don't need the fs-writeback lists" infrstructure.

Quoted 16 lines · Daniel Phillips

Quoted 16 lines

However, we already avoid the VFS writeback lists for certain filesystems for pure metadata. e.g. XFS does not use the VFS dirty inode lists for inode metadata changes. They get tracked internally by the transaction subsystem which does it's own writeback according to the requirements of journal space availability.

This is done simply by not calling mark_inode_dirty() on any metadata only change. If we want to do the same for data, then we'd simply not call mark_inode_dirty() in the data IO path. That requires a custom ->set_page_dirty method to be provided by the filesystem that didn't call

	__mark_inode_dirty(mapping->host, I_DIRTY_PAGES);

and instead did it's own thing.

Quoted 7 lines

Quoted 4 lines

So the per-superblock dirty tracking is something we can do right now, and some filesystems do it for metadata. The missing piece for data is writeback infrastructure capable of deferring to superblocks for writeback rather than BDIs....

We agree that fs-writeback inode lists are broken for anything more sophisticated than Ext2.

No, I did not say that.

I said XFS does something different for metadata changes because it has different flushing constraints and requirements than the generic code provides. That does not make the generic code broken.

Quoted 6 lines

An advantage of the patch under
consideration is that it still lets fs-writeback mostly work the way it has worked for the last few years, except for not allowing it to pick specific inodes and data pages for writeout. As far as I can see, it still balances writeout between different filesystems on the same block device pretty well.

Not really. If there are 3 superblocks on a BDI, and the dirty inode list iterates between 2 of them with lots of dirty inodes, it can starve writeback from the third until one of it's dirty inodes pops to the head of the b_io list. So it's inherently unfair from that perspective.

Changing the high level flushing to be per-superblock rather than per-BDI would enable us to control that behaviour and be much fairer to all the superblocks on a given BDI. That said, I don't really care that much about this case...

So we currently flush inodes in first dirtied first written back order when superblock is not specified in writeback work. That completely ignores the fact to which superblock inode belongs but I don't see per-sb fairness to actually make any sense when
1) flushing old data (to keep promise set in dirty_expire_centisecs)
2) flushing data to reduce number of dirty pages
And these are really the only two cases where we don't do per-sb flushing.

Now when filesystems want to do something more clever (and I can see reasons for that e.g. when journalling metadata, even more so when journalling data) I agree we need to somehow implement the above two types of writeback using per-sb flushing. Type 1) is actually pretty easy - just tell each sb to writeback dirty data upto time T. Type 2) is more difficult because that is more openended task - it seems similar to what shrinkers do but that would require us to track per sb amount of dirty pages / inodes and I'm not sure we want to add even more page counting statistics... Especially since often bdi == fs. Thoughts?

Honestly I think doing per-bdi writeback has been a major mistake. As you said it only even matters when we have filesystems on multiple partitions on a single device, and even then only in a simple setup, as soon as we use LVM or btrfs this sort of sharing stops to happen anyway. I don't even see much of a benefit except that we prevent two flushing daemons to congest a single device for that special case of multiple filesystems on partitions of the same device, and that could be solved in other ways.

In general, I do not believe that the complexity problems of soft updates, atomic writes, and related techniques can be solved by hand/manually. So my suggestion is to automatically handle the complexity problem of e.g. dependancies in a way that is comparable to a(n on-the-fly) file-system compiler so to say that works on a very large dependancy graph (having several billions of graph vertices actually). And at this point an abstraction like it is given with Featherstitch helps to feed and control this special FS compiler.

Quoted 8 lines · Jan Kara

So we currently flush inodes in first dirtied first written back order when superblock is not specified in writeback work. That completely ignores the fact to which superblock inode belongs but I don't see per-sb fairness to actually make any sense when
1) flushing old data (to keep promise set in dirty_expire_centisecs)
2) flushing data to reduce number of dirty pages
And these are really the only two cases where we don't do per-sb flushing.

Quoted 9 lines

Now when filesystems want to do something more clever (and I can see reasons for that e.g. when journalling metadata, even more so when journalling data) I agree we need to somehow implement the above two types of writeback using per-sb flushing. Type 1) is actually pretty easy - just tell each sb to writeback dirty data upto time T. Type 2) is more difficult because that is more openended task - it seems similar to what shrinkers do but that would require us to track per sb amount of dirty pages / inodes and I'm not sure we want to add even more page counting statistics... Especially since often bdi == fs. Thoughts?

Honestly I think doing per-bdi writeback has been a major mistake. As you said it only even matters when we have filesystems on multiple partitions on a single device, and even then only in a simple setup, as soon as we use LVM or btrfs this sort of sharing stops to happen anyway. I don't even see much of a benefit except that we prevent two flushing daemons to congest a single device for that special case of multiple filesystems on partitions of the same device, and that could be solved in other ways.

Quoted 8 lines · Christian Stroetmann

In general, I do not believe that the complexity problems of soft updates, atomic writes, and related techniques can be solved by hand/manually. So my suggestion is to automatically handle the complexity problem of e.g. dependancies in a way that is comparable to a(n on-the-fly) file-system compiler so to say that works on a very large dependancy graph (having several billions of graph vertices actually). And at this point an abstraction like it is given with Featherstitch helps to feed and control this special FS compiler.

Well, if you want to try to implement something like this, go for it!

I'd be very curious to see how well (a) how much CPU overhead it takes to crunch on a dependency graph with billions of vertices, and (b) how easily can it be to express these dependencies and maintainable such a dependency language would be. Sounds like a great research topic, and

I'll note the Call For Papers for FAST 2015 is out, and if you can solve these problems, it would make a great FAST 2015 submission:

https://www.usenix.org/conference/fast15/call-for-papers

Cheers,

- Ted

Quoted 1 line · Christoph Hellwig

On Tue, Jun 03, 2014 at 04:05:31PM +0200, Jan Kara wrote:

...
So I agree per-bdi / per-sb matters only in simple setups but machines
with single rotating disk with several partitions and without LVM aren't that rare AFAICT from my experience.

And I agree we went for per-bdi
flushing to avoid two threads congesting a single device leading to suboptimal IO patterns during background writeback.

So currently I'm convinced we want to go for per-sb dirty tracking. That also makes some speedups in that code noticeably simpler. I'm not

about the per-sb flushing thread - if we don't regress the multiple sb on bdi case when we just let the threads from different superblocks contend for IO, then that would be a natural thing to do. But once we have to introduce some synchronization between threads to avoid regressions, I think it might be easier to just stay with per-bdi thread which switches between superblocks.

Quoted 5 lines · Jan Kara

Quoted 1 line · Christoph Hellwig

On Tue, Jun 03, 2014 at 04:05:31PM +0200, Jan Kara wrote:

And I agree we went for per-bdi
flushing to avoid two threads congesting a single device leading to suboptimal IO patterns during background writeback.

A proposal is on the table to implement s_ops->writeback() as a per-sb operation in such a way that nothing changes in the current per-inode path. Good or bad approach?

Quoted 2 lines

So currently I'm convinced we want to go for per-sb dirty tracking. That also makes some speedups in that code noticeably simpler. I'm not

convinced

Quoted 6 lines

about the per-sb flushing thread - if we don't regress the multiple sb on bdi case when we just let the threads from different superblocks contend for IO, then that would be a natural thing to do. But once we have to introduce some synchronization between threads to avoid regressions, I think it might be easier to just stay with per-bdi thread which switches between superblocks.

Could you elaborate on the means of switching between superblocks? Do you mean a new fs-writeback path just for data=journal class filesystems, or are you suggesting changing the way all filesystems are driven?