Details

Reviewers

cem
kib

Summary

In allocbuf() newbsize is rounded up to nearest page size.
The only place where B_MALLOC is set is in vfs_nonvmio_extend(), only if newbsize is <= PAGE_SIZE / 2. But since allocbuf() rounds up to nearest page size, code which sets B_MALLOC is never run.

Since it's still desirable to have malloc bufs, this fix modifies allocbuf() to check if the buffer would be a malloc buf prior to rounding up to PAGE_SIZE, and only does so if the buffer would not be a malloc buf.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 18540
Build 18242: arc lint + arc unit

Event Timeline

darrick.freebsd_gmail.com created this revision.Aug 2 2018, 10:33 PM

Harbormaster completed remote builds in B18540: Diff 46218.Aug 2 2018, 10:33 PM

Seems like we can kill a lot more dead code than this. bufmallocadjust is now dead, the flag itself is dead, and it can be removed from various checks. bufmallocspace, maxbufmallocspace, and M_BIOBUF can all be removed, etc.

cem added inline comments.Aug 2 2018, 11:12 PM

sys/kern/vfs_bio.c
4175–4177	This routine and nonvmio_extend can probably both be inlined now that they're so short (they're each only called in one place).

Remove B_MALLOC flag entirely.

Harbormaster completed remote builds in B18543: Diff 46222.Aug 3 2018, 12:12 AM

LGTM modulo a few style nits below.

sys/kern/vfs_bio.c
4175	Ah, by "inline," I had in mind literally moving the code inside the caller. (The "inline" annotation is usually discouraged since modern (C99+) compilers are free to more or less ignore it and use their own heuristics anyway.)
sys/sys/buf.h
226 ↗	(On Diff #46222)	This should be replaced with e.g. `B_00010000` (see examples a few lines below).
sys/ufs/ffs/ffs_alloc.c
341 ↗	(On Diff #46222)	Extra parens around B_VMIO can be dropped.

CR feedback.

Harbormaster completed remote builds in B18571: Diff 46272.Aug 3 2018, 9:38 PM

Other than the one last trivial change, LGTM. Kib@, any thoughts?

sys/sys/buf.h
226 ↗	(On Diff #46222)	This one got missed, I think.

This revision is now accepted and ready to land.Aug 3 2018, 9:50 PM

CR feedback.

This revision now requires review to proceed.Aug 3 2018, 10:08 PM

Harbormaster completed remote builds in B18572: Diff 46273.Aug 3 2018, 10:08 PM

cem accepted this revision.Aug 3 2018, 10:17 PM

cem added inline comments.

sys/sys/buf.h
222–224 ↗	(On Diff #46273)	I can't tell if the whitespace matches and phab just isn't rendering it correctly, or if it doesn't match the other lines.

This revision is now accepted and ready to land.Aug 3 2018, 10:17 PM

Fix whitespace.

This revision now requires review to proceed.Aug 3 2018, 10:26 PM

Harbormaster completed remote builds in B18573: Diff 46274.Aug 3 2018, 10:26 PM

Hm, I am not sure. I agree that due to the bug, B_MALLOC buffers are never created by the current code. But, do we want to waste that many memory for small non-vmio buffers ? Isn't it better to fix creation of B_MALLOC buffers instead of removing them ? If a filesystem uses 512 or 1024 byte buffers, we do not want to multiply its memory use by a factor of 8 or 4.

It looks simple enough to set B_MALLOC if b_bufsize is zero and newbsize is less than half of the page size.

In D16573#352448, @kib wrote:

Hm, I am not sure. I agree that due to the bug, B_MALLOC buffers are never created by the current code. But, do we want to waste that many memory for small non-vmio buffers ? Isn't it better to fix creation of B_MALLOC buffers instead of removing them ? If a filesystem uses 512 or 1024 byte buffers, we do not want to multiply its memory use by a factor of 8 or 4.

It looks simple enough to set B_MALLOC if b_bufsize is zero and newbsize is less than half of the page size.

It seems that the code that prevents us from setting B_MALLOC in the case you describe is the following:

if ((bp->b_flags & B_MALLOC) == 0)
newbsize = round_page(newbsize);

Would you prefer that we not round_page() in this case?

In D16573#352870, @darrick.freebsd_gmail.com wrote:
It seems that the code that prevents us from setting B_MALLOC in the case you describe is the following:
if ((bp->b_flags & B_MALLOC) == 0)
newbsize = round_page(newbsize);
Would you prefer that we not round_page() in this case?

It is too simple. We should check the size after rounding up to DEV_BSIZE. If the size is in range for B_MALLOC, we should not round to page and create B_MALLOC buffer. Otherwise, round to the page size and create a page-backed non-vmio buffer.

Don't round new buffer size up to page size in buffer cache in cases where new buffer would be malloc buffer.

Harbormaster completed remote builds in B18623: Diff 46368.Aug 7 2018, 3:49 AM

kib added inline comments.Aug 7 2018, 8:07 AM

sys/kern/vfs_bio.c
4179	Blank line is needed before the first statement if there is no local vars defined.
4180	return (false);
4181	No need for this blank line.
4183	return (false);
4184	extra blank line, and so on for the whole function.
4223	I do not see why would check for bufmallocspace needed there.

Please update the phabricator title and summary to match the new direction of the patch when you get a chance.

CR feedback.

Harbormaster completed remote builds in B18628: Diff 46385.Aug 7 2018, 6:13 PM

darrick.freebsd_gmail.com added inline comments.Aug 7 2018, 6:14 PM

sys/kern/vfs_bio.c
4223	I suppose it's not. It just seemed clearer from a readability standpoint to call want_malloc_buf() here. If you feel strongly about it I can remove the function and just inline the if checks in both places where they're needed.

darrick.freebsd_gmail.com retitled this revision from Remove dead code in vfs_bio.c. to Fix code preventing sufficiently small buffers from being malloc buffers..Aug 7 2018, 6:14 PM

kib added inline comments.Aug 7 2018, 6:22 PM

sys/kern/vfs_bio.c
4223	I suggest to remove the space check from want_malloc_buf() and keep the check at the line 4231 inlined.

CR feedback.

Harbormaster completed remote builds in B18629: Diff 46386.Aug 7 2018, 7:47 PM

kib accepted this revision.Aug 7 2018, 8:15 PM

This revision is now accepted and ready to land.Aug 7 2018, 8:15 PM

Summary text still reflects the old patch.

sys/kern/vfs_bio.c
4187–4188	If we fail the bufmallocspace check here, should we round newbsize up to page_size? Or remove the conditional roundup below?

darrick.freebsd_gmail.com added inline comments.Aug 8 2018, 8:31 PM

sys/kern/vfs_bio.c
4187–4188	I'm fine with the roundup if that's the right thing to do.

darrick.freebsd_gmail.com edited the summary of this revision. (Show Details)Aug 8 2018, 8:33 PM

darrick.freebsd_gmail.com added inline comments.Aug 16 2018, 10:41 PM

sys/kern/vfs_bio.c
4187–4188	iirc kib explicitly wanted the bufmalloc check removed from want_malloc_buf(). That would've caused the roundup to occur when bufmallocspace >= maxbufmallocspace.

kib added inline comments.Aug 16 2018, 11:01 PM

sys/kern/vfs_bio.c
4187–4188	No, I do not want to micro-manage. I only want malloc-backed buffer to start/continue to work. Code organization is up to you.

darrick.freebsd_gmail.com added inline comments.Aug 17 2018, 5:34 PM

sys/kern/vfs_bio.c
4187–4188	Not a code organization question. Earlier the check in question was in want_malloc_buf(), so the check was done in two places: 1. before rounding up to PAGE_SIZE in the case where we don't want to set B_MALLOC, and 2. prior to setting B_MALLOC in the case where we do. But in an earlier comment you stated that "4291 I do not see why would check for bufmallocspace needed there." So I removed it and only did that check prior to setting B_MALLOC. cem is asking if we do want the check prior to rounding up to PAGE_SIZE after all.

kib added inline comments.Aug 17 2018, 6:01 PM

sys/kern/vfs_bio.c
4187–4188	I do not quite understand what you are asking. Which check below ? If we do not allocate malloc buffer, there is no point (and it is even impossible) to allocate not in quantities of PAGE_SIZE. So if the maxbufmallocspace check fails and we decided to allocate page-backed buffer, we need to round. This is the only sense that I can see in the question.

CR feedback.

This revision now requires review to proceed.Aug 17 2018, 8:09 PM

Harbormaster completed remote builds in B18909: Diff 46860.Aug 17 2018, 8:09 PM

Incorporated feedback.

Why did you moved the check for mallocspace back into the want_malloc_buf() ?

In D16573#356822, @kib wrote:

Why did you moved the check for mallocspace back into the want_malloc_buf() ?

I thought we wanted that check in both places where want want_malloc_buf() is called? I can inline it in both places if that's preferred.

In D16573#356843, @darrick.freebsd_gmail.com wrote:

In D16573#356822, @kib wrote:

Why did you moved the check for mallocspace back into the want_malloc_buf() ?

I thought we wanted that check in both places where want want_malloc_buf() is called? I can inline it in both places if that's preferred.