writeback: Don't wait for completion in writeback_inodes_sb_nr

Contrary to the comment block atop writeback_inodes_sb_nr(), we *were* calling

        wait_for_completion(&done);

which should not be done, as this is not called for data integrity sync.

Signed-off-by Curt Wohlgemuth <[email protected]>

Quoted 9 lines · Curt Wohlgemuth

Contrary to the comment block atop writeback_inodes_sb_nr(), we *were* calling

        wait_for_completion(&done);

which should not be done, as this is not called for data integrity sync.

Signed-off-by Curt Wohlgemuth <[email protected]>

The comment says it does not wait for IO to be -completed-.

The function as implemented waits for IO to be *submitted*.

This provides the callers with same blocking semantics (i.e. request queue full) as if the caller submitted the IO themselves. The code that uses this function rely on this blocking to delay the next set of operations they do until after IO has been started, so removing the completion will change their behaviour significantly.

Cheers,

Dave.
--
Dave Chinner
[email protected]

I don't quite understand this. It's true that all IO done as a result of calling wb_writeback() on this work item won't finish before the completion takes place, but sending all those pages in flight *will* take place. And that's a lot of time. To wait on this before we then call sync_inodes_sb(), and do it all over again, seems odd at best.

Pre-2.6.35 kernels would start non-integrity sync writeback and immediately return, which would seem like a reasonable "prefetch-y" thing to do, considering it's going to be immediately followed by a data integrity sync writeback operation.

Quoted 10 lines · Curt Wohlgemuth

I don't quite understand this. It's true that all IO done as a result of calling wb_writeback() on this work item won't finish before the completion takes place, but sending all those pages in flight *will* take place. And that's a lot of time. To wait on this before we then call sync_inodes_sb(), and do it all over again, seems odd at best.

Pre-2.6.35 kernels would start non-integrity sync writeback and immediately return, which would seem like a reasonable "prefetch-y" thing to do, considering it's going to be immediately followed by a data integrity sync writeback operation.

The only old kernel I have around is 2.6.16, so looking at that one for old semantics:

- it first does a wakeup_pdflush() which does a truely asynchronous writeback of everything in the system. This is replaced with a wakeup_flusher_threads(0) in 3.0, which has pretty similar sematics.
- after that things change a bit as we reordered sync quite a bit too
a) improve data integrity by properly ordering the steps of the sync and b) shared code between the global sync and per-fs sync as used by umount, sys_syncfs and a few other callers. But both do an semi-sync pass and a sync pass on per-sb writeback. For the old code it's done from the calling threads context, while the next code moved it to the flushers thread, but still waiting for it with the completion. No major change in semantics as far as I can see.

The idea of the async pass is to start writeback on as many as possible pages before actively having to wait on them. I'd agree with your assessment that the writeback_inodes_sb might not really help all that much - given that a lot of the waiting might not actually be for I/O completion but e.g. for the block level throtteling (or maybe cgroups in your case?).

For sys_sync I'm pretty sure we could simply remove the writeback_inodes_sb call and get just as good if not better performance,

but we'd still need a solution for the other sync_filesystem callers, assuming the first write actually benefits them. Maybe you can run some sys_syncfs microbenchmarks to check it? Note that doing multiple passes generally doesn't actually help live-lock avoidance either, but wu has recently done some work in that area.

Another thing we've discussed a while ago is changing sync_inodes_sb to the writeback or at least the waiting back in the calling threads context to not block the flushers threads from getting real work done.

Quoted 2 lines

For sys_sync I'm pretty sure we could simply remove the writeback_inodes_sb call and get just as good if not better performance,

Actually, it won't with current code. Because WB_SYNC_ALL writeback
currently has the peculiarity that it looks like:
for all inodes {
write all inode data
wait for inode data
}
while to achieve good performance we actually need something like
for all inodes
write all inode data
for all inodes
wait for inode data
It makes a difference in an order of magnitude when there are lots of smallish files - SLES had a bug like this so I know from user reports ;)

You mean that sync(1) would actually write the data itself? It would
certainly make some things simpler but it has its problems as well - for example sync racing with flusher thread writing back inodes can create rather bad IO pattern...

Quoted 15 lines · Jan Kara

Quoted 2 lines

For sys_sync I'm pretty sure we could simply remove the writeback_inodes_sb call and get just as good if not better performance,

Actually, it won't with current code. Because WB_SYNC_ALL writeback
currently has the peculiarity that it looks like:
for all inodes {
write all inode data
wait for inode data
}
while to achieve good performance we actually need something like
for all inodes
write all inode data
for all inodes
wait for inode data
It makes a difference in an order of magnitude when there are lots of smallish files - SLES had a bug like this so I know from user reports ;)

I don't think that's true. The WB_SYNC_ALL writeback is done using sync_inodes_sb, which operates as:

for all dirty inodes in bdi:
if inode belongs to sb
write all inode data

for all inodes in sb:
wait for inode data

we still do that in a big for each sb loop, though.

Quoted 4 lines

You mean that sync(1) would actually write the data itself? It would
certainly make some things simpler but it has its problems as well - for example sync racing with flusher thread writing back inodes can create rather bad IO pattern...

Only the second pass. The idea is that we first try to use the flusher threads for good I/O patterns, but if we can't get that to work only block the caller and not everyone. But that's just an idea so far, it would need serious benchmark. And despite what I claimed before we actually do the wait in the caller context already anyway, which already gives you the easy part of the above effect.

True but writeback_single_inode() has in it:

        if (wbc->sync_mode == WB_SYNC_ALL) {
                int err = filemap_fdatawait(mapping);
                if (ret == 0)
                        ret = err;
        }

So we end up waiting much earlier. Probably we should remove this wait
but that will need some audit I guess.

Quoted 8 lines · Jan Kara

True but writeback_single_inode() has in it:

        if (wbc->sync_mode == WB_SYNC_ALL) {
                int err = filemap_fdatawait(mapping);
                if (ret == 0)
                        ret = err;
        }

So we end up waiting much earlier. Probably we should remove this wait
but that will need some audit I guess.

Uhh, indeed. We'll need this wait for things like sync_inode, though.

Yes. Actually, specifically for filesystems like XFS which update inode
after IO completion we would need more passes to be efficient and correct:
for all inodes
fdatawrite
for all inodes
fdatawait
for all inodes
write_inode
for all inodes
wait for inode IO

But maybe this could be handled by having new WB_SYNC_ mode indicating
that writeback_single_inode() should not bother waiting (thus we'd really end up waiting in sync_inodes_sb()) and then XFS and other filesystems that need it would writeout inodes in their sync_fs() implementation (possibly using a generic helper)?

Quoted 20 lines · Christoph Hellwig

Quoted 15 lines · Jan Kara

Quoted 2 lines

For sys_sync I'm pretty sure we could simply remove the writeback_inodes_sb call and get just as good if not better performance,

  Actually, it won't with current code. Because WB_SYNC_ALL writeback currently has the peculiarity that it looks like:
  for all inodes {
    write all inode data
    wait for inode data
  }
while to achieve good performance we actually need something like for all inodes
    write all inode data
  for all inodes
    wait for inode data
It makes a difference in an order of magnitude when there are lots of smallish files - SLES had a bug like this so I know from user reports ;)

I don't think that's true.  The WB_SYNC_ALL writeback is done using sync_inodes_sb, which operates as:

Quoted 8 lines

  for all dirty inodes in bdi:
     if inode belongs to sb
        write all inode data

  for all inodes in sb:
     wait for inode data

we still do that in a big for each sb loop, though.

 True but writeback_single_inode() has in it:
       if (wbc->sync_mode == WB_SYNC_ALL) {
               int err = filemap_fdatawait(mapping);
               if (ret == 0)
                       ret = err;
       }
 So we end up waiting much earlier. Probably we should remove this wait but that will need some audit I guess.

Quoted 6 lines

Quoted 5 lines

  You mean that sync(1) would actually write the data itself? It would certainly make some things simpler but it has its problems as well - for example sync racing with flusher thread writing back inodes can create rather bad IO pattern...

Quoted 6 lines

Only the second pass.  The idea is that we first try to use the flusher threads for good I/O patterns, but if we can't get that to work only block the caller and not everyone.  But that's just an idea so far, it would need serious benchmark.  And despite what I claimed before we actually do the wait in the caller context already anyway, which already gives you the easy part of the above effect.

 Modulo the writeback_single_inode() wait. But if that is dealt with I agree.

                                                               Honza --
Jan Kara <[email protected]>
SUSE Labs, CR

One other issue I have with sync as it's structured is that we don't do a WB_SYNC_ALL pass on any inode that's only associated with a block device, and not on a mounted filesystem. Blockdev mounts are pseudo-mounts, and are explicitly skipped in __sync_filesystem(). So if you've written directly to a block device and do a sync, the only pass over the pages for this inode are via the wakeup_flusher_threads() -- which operates on a BDI, regardless of the superblock, and uses WB_SYNC_NONE.

All the sync_filesystem() calls are per-sb, not per-BDI, and they'll exclude pseudo-superblocks.

I've seen cases in our modified kernels here at Google in which lilo/shutdown failed because of a lack of WB_SYNC_ALL writeback for /dev/sda (though I haven't been able to come up with a consistent test case, nor reproduce this on an upstream kernel).

Quoted 29 lines · Jan Kara

Quoted 20 lines · Christoph Hellwig

Quoted 15 lines · Jan Kara

Quoted 2 lines

For sys_sync I'm pretty sure we could simply remove the writeback_inodes_sb call and get just as good if not better performance,

  Actually, it won't with current code. Because WB_SYNC_ALL writeback currently has the peculiarity that it looks like:
  for all inodes {
    write all inode data
    wait for inode data
  }
while to achieve good performance we actually need something like for all inodes
    write all inode data
  for all inodes
    wait for inode data
It makes a difference in an order of magnitude when there are lots of smallish files - SLES had a bug like this so I know from user reports ;)

I don't think that's true.  The WB_SYNC_ALL writeback is done using sync_inodes_sb, which operates as:

Quoted 8 lines

  for all dirty inodes in bdi:
     if inode belongs to sb
        write all inode data

  for all inodes in sb:
     wait for inode data

we still do that in a big for each sb loop, though.

Quoted 8 lines

 True but writeback_single_inode() has in it:
       if (wbc->sync_mode == WB_SYNC_ALL) {
               int err = filemap_fdatawait(mapping);
               if (ret == 0)
                       ret = err;
       }
 So we end up waiting much earlier. Probably we should remove this wait but that will need some audit I guess.

So today for WB_SYNC_ALL from sync_inodes_sb(), we do:
- queue a work item; this will
- write all dirty inodes in a sb
- write one inode's pages
- wait on all inode's pages
- wait for the work item
- wait on all inodes in the sb (wait_sb_inodes)

I guess the point of wait_sb_inodes() is to wait on all inodes that were written in a previous writeback pass.

One other issue I have with sync as it's structured is that we don't do a WB_SYNC_ALL pass on any inode that's only associated with a block device, and not on a mounted filesystem. Blockdev mounts are pseudo-mounts, and are explicitly skipped in __sync_filesystem(). So if you've written directly to a block device and do a sync, the only pass over the pages for this inode are via the wakeup_flusher_threads() -- which operates on a BDI, regardless of the superblock, and uses WB_SYNC_NONE.

All the sync_filesystem() calls are per-sb, not per-BDI, and they'll exclude pseudo-superblocks.

I've seen cases in our modified kernels here at Google in which lilo/shutdown failed because of a lack of WB_SYNC_ALL writeback for /dev/sda (though I haven't been able to come up with a consistent test case, nor reproduce this on an upstream kernel).

 Hi Curt,

Quoted 47 lines · Curt Wohlgemuth

Quoted 29 lines · Jan Kara

Quoted 20 lines · Christoph Hellwig

Quoted 15 lines · Jan Kara

Quoted 2 lines

For sys_sync I'm pretty sure we could simply remove the writeback_inodes_sb call and get just as good if not better performance,

  Actually, it won't with current code. Because WB_SYNC_ALL writeback currently has the peculiarity that it looks like:
  for all inodes {
    write all inode data
    wait for inode data
  }
while to achieve good performance we actually need something like for all inodes
    write all inode data
  for all inodes
    wait for inode data
It makes a difference in an order of magnitude when there are lots of smallish files - SLES had a bug like this so I know from user reports ;)

I don't think that's true.  The WB_SYNC_ALL writeback is done using sync_inodes_sb, which operates as:

Quoted 8 lines

  for all dirty inodes in bdi:
     if inode belongs to sb
        write all inode data

  for all inodes in sb:
     wait for inode data

we still do that in a big for each sb loop, though.

Quoted 8 lines

 True but writeback_single_inode() has in it:
       if (wbc->sync_mode == WB_SYNC_ALL) {
               int err = filemap_fdatawait(mapping);
               if (ret == 0)
                       ret = err;
       }
 So we end up waiting much earlier. Probably we should remove this wait but that will need some audit I guess.

So today for WB_SYNC_ALL from sync_inodes_sb(), we do:
  - queue a work item; this will
    - write all dirty inodes in a sb
      - write one inode's pages
      - wait on all inode's pages
  - wait for the work item
  - wait on all inodes in the sb (wait_sb_inodes)

Quoted 2 lines

I guess the point of wait_sb_inodes() is to wait on all inodes that were written in a previous writeback pass.

 Yes. As we discussed with Christoph, waiting for each inode could be avoided in principle but we have to be careful not to break data consistency guarantees in some other path calling writeback_single_inode()...

Quoted 12 lines

One other issue I have with sync as it's structured is that we don't do a WB_SYNC_ALL pass on any inode that's only associated with a block device, and not on a mounted filesystem.  Blockdev mounts are pseudo-mounts, and are explicitly skipped in __sync_filesystem().  So if you've written directly to a block device and do a sync, the only pass over the pages for this inode are via the wakeup_flusher_threads() -- which operates on a BDI, regardless of the superblock, and uses WB_SYNC_NONE.

All the sync_filesystem() calls are per-sb, not per-BDI, and they'll exclude pseudo-superblocks.

Quoted 4 lines

I've seen cases in our modified kernels here at Google in which lilo/shutdown failed because of a lack of WB_SYNC_ALL writeback for /dev/sda (though I haven't been able to come up with a consistent test case, nor reproduce this on an upstream kernel).

 Ho hum, I wonder what would be the cleanest way to fix this. I guess we could make an exception for 'bdev' superblock in the code but it's ugly.

                                                               Honza --
Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 12 lines

Quoted 12 lines

One other issue I have with sync as it's structured is that we don't do a WB_SYNC_ALL pass on any inode that's only associated with a block device, and not on a mounted filesystem.  Blockdev mounts are pseudo-mounts, and are explicitly skipped in __sync_filesystem().  So if you've written directly to a block device and do a sync, the only pass over the pages for this inode are via the wakeup_flusher_threads() -- which operates on a BDI, regardless of the superblock, and uses WB_SYNC_NONE.

All the sync_filesystem() calls are per-sb, not per-BDI, and they'll exclude pseudo-superblocks.

Quoted 6 lines

Quoted 4 lines

I've seen cases in our modified kernels here at Google in which lilo/shutdown failed because of a lack of WB_SYNC_ALL writeback for /dev/sda (though I haven't been able to come up with a consistent test case, nor reproduce this on an upstream kernel).

 Ho hum, I wonder what would be the cleanest way to fix this. I guess we could make an exception for 'bdev' superblock in the code but it's ugly.

Yes, it's ugly.

You could declare that sync(2) is just doing it's job, that it's not designed to write dirty pages from devices that don't have filesystems mounted on them; that's what Christoph seems to be saying, and it's what the man pages for sync(2) that I've seen say as well. But everybody I've talked to about this is surprised, so you could declare it a bug as well :-) .

It seems to me that sys_sync *could* just dispense with iterating over the superblocks, and just iterate over the BDIs, just as wakeup_flusher_threads() does. I.e., just do two passes over all BDIs -- one with WB_SYNC_NONE, and one with WB_SYNC_ALL. Well, not quite. It would still have to go to each SB and call the quota_sync and sync_fs callbacks, but those should be cheap.

And yes, this would make sys_sync and sys_syncfs more different than they are today.

Quoted 28 lines · Curt Wohlgemuth

Quoted 12 lines

Quoted 12 lines

One other issue I have with sync as it's structured is that we don't do a WB_SYNC_ALL pass on any inode that's only associated with a block device, and not on a mounted filesystem.  Blockdev mounts are pseudo-mounts, and are explicitly skipped in __sync_filesystem().  So if you've written directly to a block device and do a sync, the only pass over the pages for this inode are via the wakeup_flusher_threads() -- which operates on a BDI, regardless of the superblock, and uses WB_SYNC_NONE.

All the sync_filesystem() calls are per-sb, not per-BDI, and they'll exclude pseudo-superblocks.

Quoted 6 lines

Quoted 4 lines

I've seen cases in our modified kernels here at Google in which lilo/shutdown failed because of a lack of WB_SYNC_ALL writeback for /dev/sda (though I haven't been able to come up with a consistent test case, nor reproduce this on an upstream kernel).

 Ho hum, I wonder what would be the cleanest way to fix this. I guess we could make an exception for 'bdev' superblock in the code but it's ugly.

Yes, it's ugly.

You could declare that sync(2) is just doing it's job, that it's not designed to write dirty pages from devices that don't have filesystems mounted on them; that's what Christoph seems to be saying, and it's what the man pages for sync(2) that I've seen say as well.  But everybody I've talked to about this is surprised, so you could declare it a bug as well :-) .

Quoted 6 lines

It seems to me that sys_sync *could* just dispense with iterating over the superblocks, and just iterate over the BDIs, just as wakeup_flusher_threads() does.  I.e., just do two passes over all BDIs -- one with WB_SYNC_NONE, and one with WB_SYNC_ALL.  Well, not quite. It would still have to go to each SB and call the quota_sync and sync_fs callbacks, but those should be cheap.

 Well, they're not exactly cheap (journaling filesystems have to force transaction commits and wait for them) but that does not really matter. The real problem is that to wait for IO completion you need some list of inodes you want to wait for and you can currently get such list only from superblock.

Quoted 3 lines

And yes, this would make sys_sync and sys_syncfs more different than they are today.

                                                               Honza --
Jan Kara <[email protected]>
SUSE Labs, CR

Well, they're not exactly cheap (journaling filesystems have to force
transaction commits and wait for them) but that does not really matter. The real problem is that to wait for IO completion you need some list of inodes you want to wait for and you can currently get such list only from superblock.

Quoted 5 lines · Jan Kara

Well, they're not exactly cheap (journaling filesystems have to force
transaction commits and wait for them) but that does not really matter. The real problem is that to wait for IO completion you need some list of inodes you want to wait for and you can currently get such list only from superblock.

All block device inodes sit on blockdev_superblock, we got rid of inodes without a superblock long time ago.

Quoted 2 lines

All block device inodes sit on blockdev_superblock, we got rid of inodes without a superblock long time ago.

Sure, we can easily iterate also blockdev_superblock. What I meant is
that blockdev_superblock will need a special handling since we otherwise ignore pseudo superblocks...

Quoted 5 lines · Jan Kara

Quoted 2 lines

All block device inodes sit on blockdev_superblock, we got rid of inodes without a superblock long time ago.

Sure, we can easily iterate also blockdev_superblock. What I meant is
that blockdev_superblock will need a special handling since we otherwise ignore pseudo superblocks...

Pseudo superblocks aren't ignored. They are added to super_blocks like all others, and iterate_supers doesn't skip over them. The problem is that blockdev_superblock doesn't have a proper s_bdi set, and thus gets skipped over by __sync_filesystem.

From 2834bd2727c93055bb7373d8849492044f70c530 Mon Sep 17 00:00:00 2001

Quoted 11 lines · Christoph Hellwig

Quoted 5 lines · Jan Kara

Quoted 2 lines

All block device inodes sit on blockdev_superblock, we got rid of inodes without a superblock long time ago.

  Sure, we can easily iterate also blockdev_superblock. What I meant is that blockdev_superblock will need a special handling since we otherwise ignore pseudo superblocks...

Pseudo superblocks aren't ignored.  They are added to super_blocks like all others, and iterate_supers doesn't skip over them.  The problem is that blockdev_superblock doesn't have a proper s_bdi set, and thus gets skipped over by __sync_filesystem.

 Yes. But even if it was not skipped writeback_inodes_sb() doesn't have one flusher thread to kick to actually do the writeout (since each inode on blockdev_superblock belongs to a different bdi). So it's perfectly fine we skip blockdev_superblock.

 If we want to fix the problem something like attached patch should do. Comments?

                                                               Honza

PS: While testing the patch, I've noticed that block device can have any dirty data only if it is still open (__blkdev_put() writes all dirty pages) so that somehow limits how much people can be burned by sync not writing out block devices...
--
Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 17 lines · Jan Kara

Quoted 11 lines · Christoph Hellwig

Quoted 5 lines · Jan Kara

Quoted 2 lines

All block device inodes sit on blockdev_superblock, we got rid of inodes without a superblock long time ago.

  Sure, we can easily iterate also blockdev_superblock. What I meant is that blockdev_superblock will need a special handling since we otherwise ignore pseudo superblocks...

Pseudo superblocks aren't ignored.  They are added to super_blocks like all others, and iterate_supers doesn't skip over them.  The problem is that blockdev_superblock doesn't have a proper s_bdi set, and thus gets skipped over by __sync_filesystem.

 Yes. But even if it was not skipped writeback_inodes_sb() doesn't have one flusher thread to kick to actually do the writeout (since each inode on blockdev_superblock belongs to a different bdi). So it's perfectly fine we skip blockdev_superblock.

Quoted 2 lines

 If we want to fix the problem something like attached patch should do. Comments?

Your patch looks good to me, in that it does hit all the bdevs with both WB_SYNC_NONE and SYNC_ALL. However, I still say that the call to wakeup_flusher_threads() in sys_sync() is superfluous, at least as long as writeback_inodes_sb() waits for completion of the work item that it enqueues.

From 7d17da2ceed603a3cb013e06a2158e56029043a8 Mon Sep 17 00:00:00 2001

Actually, it's the other way around writeback_inodes_sb() is superfluous
because of wakeup_flusher_threads(). So something like attached patch could improve sync times (especially in presence of other IO). So far I have only checked that sync times look reasonable with it but didn't really compare them with original kernel...

Quoted 5 lines · Jan Kara

Actually, it's the other way around writeback_inodes_sb() is superfluous
because of wakeup_flusher_threads(). So something like attached patch could improve sync times (especially in presence of other IO). So far I have only checked that sync times look reasonable with it but didn't really compare them with original kernel...

This changes the order in which ->quota_sync is called relatively to other operations, see my other mail about it. Also the code gets really confusing at this point, I think you're better of stopping to try to shared code between syncfs, umount & co and sys_sync with these bits.

You're also skipping the ->sync_fs and quotasync calls the first round.

I know for XFS sync_fs without wait is already mostly a no-op, but we'll need to benchmark and document this change, and apply it to the non-sync caller as well.

Quoted 1 line

You're also skipping the ->sync_fs and quotasync calls the first round.

Well, kind of. Since writeback is running asynchronously, we have no way
to call ->sync_fs(sb, 0) just after async inode writeback is done (and it doesn't really make sense to call it before that moment). So we call synchronous inode writeback first and only then ->sync_fs().

Where would you like to document the change?

Your patch looks good to me, in that it does hit all the bdevs with both WB_SYNC_NONE and SYNC_ALL. However, I still say that the call to wakeup_flusher_threads() in sys_sync() is superfluous, at least as long as writeback_inodes_sb() waits for completion of the work item that it enqueues.

fs/sync.c +1 -8

-static int __sync_filesystem(struct super_block *sb, int wait)
+static void __sync_filesystem(struct super_block *sb, int wait)
 {
-	/*
-	 * This should be safe, as we require bdi backing to actually
-	 * write out data in the first place
-	 */
-	if (sb->s_bdi == &noop_backing_dev_info)
-		return 0;
-
 	if (sb->s_qcop && sb->s_qcop->quota_sync)
 		sb->s_qcop->quota_sync(sb, -1, wait);

fs/sync.c +24 -0

+static void sync_all_bdevs(int wait)
+{
+	struct inode *inode, *old_inode = NULL;
+
+	spin_lock(&inode_sb_list_lock);
+	list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
+		struct address_space *mapping = inode->i_mapping;
+
+		spin_lock(&inode->i_lock);
+		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
+		    mapping->nrpages == 0) {
+			spin_unlock(&inode->i_lock);
+			continue;
+		}
+		__iget(inode);
+		spin_unlock(&inode->i_lock);
+		spin_unlock(&inode_sb_list_lock);
+		/*
+		 * We hold a reference to 'inode' so it couldn't have been
+		 * removed from s_inodes list while we dropped the
+		 * inode_sb_list_lock.  We cannot iput the inode now as we can
+		 * be holding the last reference and we cannot iput it under
+		 * inode_sb_list_lock. So we keep the reference and iput it
+		 * later.

Quoted 15 lines · Jan Kara

fs/sync.c +1 -8

-static int __sync_filesystem(struct super_block *sb, int wait)
+static void __sync_filesystem(struct super_block *sb, int wait)
 {
-	/*
-	 * This should be safe, as we require bdi backing to actually
-	 * write out data in the first place
-	 */
-	if (sb->s_bdi == &noop_backing_dev_info)
-		return 0;
-
 	if (sb->s_qcop && sb->s_qcop->quota_sync)
 		sb->s_qcop->quota_sync(sb, -1, wait);

@@ -47,7 +40,6 @@ static int __sync_filesystem(struct super_block *sb, int wait)
 

fs/sync.c static int __sync_filesystem(struct super_block *sb, int wait) · line 50

Quoted 3 lines

fs/sync.c +0 -1

 	if (sb->s_op->sync_fs)
 		sb->s_op->sync_fs(sb, wait);
-	return __sync_blockdev(sb->s_bdev, wait);

Removing this call breaks sys_syncfs and similar semantics on filesystem just pushing metadata into buffers in ->write_inode or ->sync_fs and then expecting the caller to write them out. This list of filesystem includes ext2 and in general most filesystems without journaling or similar technics.

I'm perfectly fine with pushing the sync_blockdev call into the filesystem for these, but we'll need a way to handle them.

fs/sync.c int sync_filesystem(struct super_block *sb) · line 71

Quoted 6 lines

fs/sync.c +6 -0

+	/*
+	 * This should be safe, as we require bdi backing to actually
+	 * write out data in the first place.
+	 */
+	if (sb->s_bdi == &noop_backing_dev_info)
+		return 0;

There's really no reason to do that check early. There's nothing no reason to not have a filesystem that doesn't use the writeback code, but still has a ->sync_fs method. IMHO this check should move into sync_inodes_sb/writeback_inodes_sb.

Quoted 30 lines

fs/sync.c +30 -0

+static void sync_all_bdevs(int wait)
+{
+	struct inode *inode, *old_inode = NULL;
+
+	spin_lock(&inode_sb_list_lock);
+	list_for_each_entry(inode, &blockdev_superblock->s_inodes, i_sb_list) {
+		struct address_space *mapping = inode->i_mapping;
+
+		spin_lock(&inode->i_lock);
+		if (inode->i_state & (I_FREEING|I_WILL_FREE|I_NEW) ||
+		    mapping->nrpages == 0) {
+			spin_unlock(&inode->i_lock);
+			continue;
+		}
+		__iget(inode);
+		spin_unlock(&inode->i_lock);
+		spin_unlock(&inode_sb_list_lock);
+		/*
+		 * We hold a reference to 'inode' so it couldn't have been
+		 * removed from s_inodes list while we dropped the
+		 * inode_sb_list_lock.  We cannot iput the inode now as we can
+		 * be holding the last reference and we cannot iput it under
+		 * inode_sb_list_lock. So we keep the reference and iput it
+		 * later.
+		 */
+		iput(old_inode);
+		old_inode = inode;
+
+		__sync_blockdev(I_BDEV(inode), wait);
+

fs/sync.c static void sync_filesystems(int wait) · line 122

Quoted 4 lines

fs/sync.c +4 -0

+		spin_lock(&inode_sb_list_lock);
+	}
+	spin_unlock(&inode_sb_list_lock);
+	iput(old_inode);

At which point we could fold this code into a blkdev_sync_fs method for now. Long term we'll need to support multiple BDIs per SB anyway, at which point the code can go away again.

Quoted 5 lines

Removing this call breaks sys_syncfs and similar semantics on filesystem just pushing metadata into buffers in ->write_inode or ->sync_fs and then expecting the caller to write them out. This list of filesystem includes ext2 and in general most filesystems without journaling or similar technics.

Note that for sys_syncfs() the __sync_blockdev() has just moved from
__sync_filesystem() to sync_filesystem(). So nothing should change.

Quoted 2 lines

I'm perfectly fine with pushing the sync_blockdev call into the filesystem for these, but we'll need a way to handle them.

Yes, calling sync_blockdev() from sync_fs() might actually make sence
(e.g. ext3 and ext4 don't need it) but I didn't want to go that far in this patch.

Quoted 3 lines

At which point we could fold this code into a blkdev_sync_fs method for now. Long term we'll need to support multiple BDIs per SB anyway, at which point the code can go away again.

Ah, I had to think a bit before I understood what you mean :). It's kind
of elegant but also slightly subtle (it's not immediately obvious how blockdevs are synced during sync when you look at the code). Umm, and you don't have any guarantee in which order superblocks are on the sb list so you could sync block devices before some filesystems are finished. So I don't think using blkdev_sync_fs() is a good idea after all.

Quoted 10 lines · Curt Wohlgemuth

I don't quite understand this.  It's true that all IO done as a result of calling wb_writeback() on this work item won't finish before the completion takes place, but sending all those pages in flight *will* take place.  And that's a lot of time.  To wait on this before we then call sync_inodes_sb(), and do it all over again, seems odd at best.

Pre-2.6.35 kernels would start non-integrity sync writeback and immediately return, which would seem like a reasonable "prefetch-y" thing to do, considering it's going to be immediately followed by a data integrity sync writeback operation.

The only old kernel I have around is 2.6.16, so looking at that one for old semantics:

 - it first does a wakeup_pdflush() which does a truely asynchronous writeback of everything in the system.  This is replaced with a wakeup_flusher_threads(0) in 3.0, which has pretty similar sematics.
 - after that things change a bit as we reordered sync quite a bit too
  a) improve data integrity by properly ordering the steps of the sync and b) shared code between the global sync and per-fs sync as used by umount, sys_syncfs and a few other callers.  But both do an semi-sync pass and a sync pass on per-sb writeback.  For the old code it's done from the calling threads context, while the next code moved it to the flushers thread, but still waiting for it with the completion.  No major change in semantics as far as I can see.

The idea of the async pass is to start writeback on as many as possible pages before actively having to wait on them.  I'd agree with your assessment that the writeback_inodes_sb might not really help all that much - given that a lot of the waiting might not actually be for I/O completion but e.g. for the block level throtteling (or maybe cgroups in your case?).

For sys_sync I'm pretty sure we could simply remove the writeback_inodes_sb call and get just as good if not better performance, but we'd still need a solution for the other sync_filesystem callers, assuming the first write actually benefits them.  Maybe you can run some sys_syncfs microbenchmarks to check it?  Note that doing multiple passes generally doesn't actually help live-lock avoidance either, but wu has recently done some work in that area.

Another thing we've discussed a while ago is changing sync_inodes_sb to the writeback or at least the waiting back in the calling threads context to not block the flushers threads from getting real work done.

The semantics did change between 2.6.34 and 2.6.35 though. When the work item queue was introduced in 2.6.32, the semantics changed from what you describe above to what's present through 2.6.34:
writeback_inodes_sb() would enqueue a work item, and return. Your commit 83ba7b07 ("writeback: simplify the write back thread queue") added the wait_for_completion() call, putting the semantics back to where they were pre-2.6.32.

Though one additional change between the old way (pre-2.6.32) and
today: with the old kernel, the pdflush thread would operate concurrently with the first (and second?) sync path through writeback.
Today of course, they're *all* serialized. So really a call to
sys_sync() will enqueue 3 work items -- one from wakeup_flusher_threads(), one from writeback_inodes_sb(), and one from sync_inodes_sb().

Quoted 7 lines · Curt Wohlgemuth

The semantics did change between 2.6.34 and 2.6.35 though.  When the work item queue was introduced in 2.6.32, the semantics changed from what you describe above to what's present through 2.6.34:
writeback_inodes_sb() would enqueue a work item, and return.  Your commit 83ba7b07 ("writeback: simplify the write back thread queue") added the wait_for_completion() call, putting the semantics back to where they were pre-2.6.32.

Yes.  The kernels inbetween had that nasty writeback vs umount races that we could trigger quite often.

Quoted 7 lines

Though one additional change between the old way (pre-2.6.32) and
today:  with the old kernel, the pdflush thread would operate concurrently with the first (and second?) sync path through writeback. Today of course, they're *all* serialized.  So really a call to sys_sync() will enqueue 3 work items -- one from wakeup_flusher_threads(), one from writeback_inodes_sb(), and one from sync_inodes_sb().

Yes.  And having WB_SYNC_NONE items from both wakeup_flusher_threads vs writeback_inodes_sb is plain stupid.  The initial conversion to the new sync_filesystem scheme had removed the wakeup_flusher_threads call, but that caused a huge regression in some benchmark.

As mentioned before Wu was working on some code to introduce tagging so that the WB_SYNC_ALL call won't start writing pages dirtied after the sync call, which should help with your issue.  Although to completely solve it we really need to get down to just two passes.

I can definitely see how tagging with the start of sync would be helpful; also how going from three to two passes seems like a no-brainer.

But what's the real point of doing even two passes -- one SYNC_NONE followed by one SYNC_ALL? Is it try to get through as many inodes as possible before we potentially lock up by waiting on an inode on an unavailable device?

The comment says it does not wait for IO to be -completed-.

The function as implemented waits for IO to be *submitted*.

This provides the callers with same blocking semantics (i.e. request queue full) as if the caller submitted the IO themselves. The code that uses this function rely on this blocking to delay the next set of operations they do until after IO has been started, so removing the completion will change their behaviour significantly.