Buffer pager for FFS.

[kib]

Summary

This is mostly optional FFS pager which, comparing with the generic local fs pager from vnode_pager.c, correctly operates on FFS volumes which underlying block device has block size larger than the machine page size.

Main issue there is that vnode_pager.c cannot easily align page-in requests on the block size. It would need to grab and busy pages outside the page-in run, and handle valid pages in the run extensions. Buffer cache/FFS already handle these problems, so simply bread()-ing the corresponding buffers provides the pager functionality almost for free. In particular, the most delicate issue with valid pages substitutions by bogus pages is avoided.

The only problematic issue is that exclusively busy state, which is assumed by the vm_pager_getpages() interface, is incompatible with the VMIO buffer cache desire to shared-busy pages. Most of the code handles the downgrade (trivial) before reading buffers, and less trivial upgrade from the shared-busy to excl-busy.

I release buffers after the read, if possible (see the check for b_dep and B_RELBUF flag setting) to somewhat ease the buffer cache pressure, this is probably not strictly needed.

Test Plan

Patch was tested by Peter on both 8K and normal volumes, for later the sysctl vfs.ffs.use_buf_pager = 1 was set. In fact I might benchmark this to consider changing default to use_buf_pager = 1 for HEAD (but did not, yet).

[markj]

Did you consider some mechanism (e.g. new buf flag) to indicate to the buffer cache that the pages are already (exclusive-)busied? Is there some reason this wouldn't be sufficient?

What FFS-specific properties is this code relying on?

sys/ufs/ffs/ffs_vnops.c

123

Looks like these were sorted before.

1811

Isn't "ma" a more common name for this?

1859

Under what circumstances can m->valid == 0 be true here?

[kib]

In D8198#170382, @markj wrote:

Did you consider some mechanism (e.g. new buf flag) to indicate to the buffer cache that the pages are already (exclusive-)busied? Is there some reason this wouldn't be sufficient?

Yes, I did considered, but not for too long. I decided that the buffer cache modifications would be needed for this code, and went with the downgrade. I wanted the pager be as little intrusive as possible.

Note that just single flag specifying that the buffer pages are excl-busy is not enough. Page-in request typically does not include all buffer pages in the run, so only some pages are excl-busy by the current thread. This makes the 'buffers with some pages excl-busy' complicated.

If not the soft-busy mechanism bugs, this patch would consist only of the additional code in ffs_vnops.c.

What FFS-specific properties is this code relying on?

At very least, the b_deps check is pure FFS specific. Also, the concept of the um_devvp -- device vnode of the mount, and the way to fetch the block size of that device is also FFS-specific. Yes, with some minor refactoring the pager should work for msdosfs at least, and might be for cd9660. But this is not the straight plug-in due to the above details, which needs to be filled there as well.

I will fix style issues you pointed out tomorrow, thanks.

sys/ufs/ffs/ffs_vnops.c

1859

The page can be invalidated while the object lock is not owned. Other thread might mmap the page after bread() marked it as valid, and invalidate. I believe that the soft-busy state of the page would not prevent the scenario.

[kib]

Style.

[mckusick]

This looks good.

Unless it causes a regression in performance, I think that it should be enabled in HEAD,

[alc]

sys/ufs/ffs/ffs_vnops.c

1896

Shouldn't you be unlocking and relocking the object here?

[kib]

Unlock the object around VM_WAIT.
Restructure the busy loop to have the control flow less convoluted.

[alc]

sys/ufs/ffs/ffs_vnops.c

1906

"Since the pages ..."

1907

"... neither the buffer nor the object lock was held ..."

1908

"Recheck the valid ..."

[kib]

Grammar fixes.

[mckusick]

I concur with the changes suggested by alc@

[pho]

Ran an old dbench test with D8197+8 applied + vfs.ffs.use_buf_pager=1 :

$ procs=4 ../compare.sh dbench.r307234.2016-10-13.20161014.050428.log dbench.r307234M.2016-10-13.20161014.101527.log
Ministat for 4 procs:
x r307234
+ r307234M
+------------------------------------------------------------+
|+ + x x x x x + ++ |
| |__________|________AM______A|______________M___________||
+------------------------------------------------------------+
N Min Max Median Avg Stddev
x 5 598.833 602.65 600.993 600.9392 1.5471662
+ 5 596.953 606.203 605.05 602.3048 4.7793623
No difference proven at 95.0% confidence
$

[kib]

In D8198#171414, @pho wrote:

Ran an old dbench test with D8197+8 applied + vfs.ffs.use_buf_pager=1 :
No difference proven at 95.0% confidence

Thank you, this is interesting.

If measuring the speed, a possible curious experiment is to do the same measurement with removed line 1865

bp->b_flags |= B_RELBUF;

The system may just deadlock without it, but if not, the removal might make some difference in the benchmark.

[alc]

sys/ufs/ffs/ffs_vnops.c

1822

The vnode lock is ensuring that vnp_size doesn't concurrently change, yes?

1846

The busy status of the page is ensuring that the page's valid field doesn't change, yes? (Normally, we would lock the object to test the page's valid field.)

1889

Was there a reason not to use vm_page_grab() here?

1912

I found this hard to read. Do you mean, "... and partial validity for the page at index count - 1 could mean ..."

1914

"..., so we must restart ..."

[kib]

sys/ufs/ffs/ffs_vnops.c

1822

Sure. Also, Peter' tests show that the issue with the parallel truncation is fixed.

1846

It does not. Shared-busy pages can be invalidated. But the test skips useless creation of the buffer for most typical case when invalidation is not used. The shared->excl upgrade loop at the end would catch the case if we raced, in the reliable way (protected by the object lock).

1889

Initially the code was structured differently, and vm_page_grab() was not suitable. Now indeed it could be used.

But I started considering that I must also drop the vnode lock around VM_WAIT, after your pointed my bug with the object lock. I probably will add the vnode unlocking around the VM_WAIT, then vm_page_grab() would become unsuitable again.

[kib]

From Peter' mail:

With this I see an improvement in throughput:

$ ../compare.sh dbench.r307234.2016-10-13.20161014.050428.log dbench.r307234M.2016-10-13.20161014.160711.log
Ministat for 4 procs:
x r307234
+ r307234M
+------------------------------------------------------------+
|                                         +                  |
|x xx xx                                 ++        +        +|
| |_A__|                               |__M____A________|    |
+------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x   5       598.833        602.65       600.993      600.9392     1.5471662
+   5       625.198       637.268       625.772      629.0804      5.268211
Difference at 95.0% confidence
 28.1412 +/- 5.66242
 4.68287% +/- 0.942261%
 (Student's t, pooled s = 3.88251)
$ 

My old dbench setup runs on a async mounted swap backed memory disk, ...

There was no difference in the buildworld times, I asked mjg@ to measure buildworld with this on pig1.

[kib]

Switch to vm_page_grab(). Buffer cache does not drop vnode lock around page allocations, page-in can safely do same.

Comment-out B_RELBUF and explain why it might be useful, in the comment.

Update comments.

[kib]

With mjg@ testing on pig1 (40 cores), the impact of the change on buildworld:

The differnce is real-time is marginal.
sys-time is the same.

(statistically significant).

In other words, there is no regressions with the buildworld load, confirmed both by pho@ and mjg@.

[alc]

In addition to adding a comment concerning the unsynchronized "m->valid" accesses, I would also suggest turning a portion of the summary into a comment at the top of this function. Otherwise, I'm happy with this change.

sys/ufs/ffs/ffs_vnops.c

1846

I think that your response to my question should be turned into a comment in the code.

[mckusick]

Given the comparable performance measures by pho@ and mjg@, I suggest that this change be enabled by default.

I suggest that rather than commenting out the B_RELBUF that it be enabled by a sysctl so that folks (like Netflix) that want to keep down the pressure on the buffer cache can easily do so.

[kib]

Add requested comments.
Disable racy assert (but useful for debugging in the controlled env).
Add a knob to enable B_RELBUF.
Enable buf pager by default.

[kib]

Alan, please note that D8197 is the prerequisite for this code.

[mckusick]

Thanks for the additions.Looks good to go.

[alc]

sys/ufs/ffs/ffs_vnops.c

1802

"In contrast to the generic ...

1804

"... size. The buffer

1806

"... does the necessary ...

1807

"replacements ...

1810

"... that the exclusive ...

1812

"... buffer cache's desire ...

1813

"pages. This function performs a trivial downgrade of the pages' state before

1814

"... and a less ...

1815

"the excl-busy state after the read.

[alc]

sys/ufs/ffs/ffs_vnops.c

1869

"... busy and the object ...

1870

"... which together allow for the pages' invalidation. ...

1872

"... for the most ...

1875

"... in a reliable ...

[kib]

Language fixes.

[mckusick]

Make it so.