Page MenuHomeFreeBSD

Fix code preventing sufficiently small buffers from being malloc buffers.
AcceptedPublic

Authored by darrick.freebsd_gmail.com on Aug 2 2018, 10:33 PM.

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 19937
Build 19454: arc lint + arc unit

Event Timeline

cem added a comment.Aug 2 2018, 11:11 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.
cem added a comment.Aug 3 2018, 1:39 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.

cem accepted this revision.Aug 3 2018, 9:50 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
This revision now requires review to proceed.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
This revision now requires review to proceed.Aug 3 2018, 10:26 PM
kib added a comment.Aug 4 2018, 3:50 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?

kib added a comment.Aug 6 2018, 8:29 PM

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.

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.

4277

I do not see why would check for bufmallocspace needed there.

cem added a comment.Aug 7 2018, 4:20 PM

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

sys/kern/vfs_bio.c
4277

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
4277

I suggest to remove the space check from want_malloc_buf() and keep the check at the line 4231 inlined.

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
cem added a comment.Aug 7 2018, 8:41 PM

Summary text still reflects the old patch.

sys/kern/vfs_bio.c
4216–4224

If we fail the bufmallocspace check here, should we round newbsize up to page_size? Or remove the conditional roundup below?

sys/kern/vfs_bio.c
4216–4224

I'm fine with the roundup if that's the right thing to do.

sys/kern/vfs_bio.c
4216–4224

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
4216–4224

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.

sys/kern/vfs_bio.c
4216–4224

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
4216–4224

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.

This revision now requires review to proceed.Aug 17 2018, 8:09 PM
kib added a comment.Aug 17 2018, 8:33 PM

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

darrick.freebsd_gmail.com marked an inline comment as done.Aug 17 2018, 9:14 PM
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.

kib accepted this revision.Aug 18 2018, 10:54 AM
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.

We already discussed that some time ago. But lets move on.

I think whatever is in review right now, is fine.

This revision is now accepted and ready to land.Aug 18 2018, 10:54 AM
  • Use patch as suggested by cem instead. Move roundup within vfs_nonvmio_* instead of using check in allocbuf().
This revision now requires review to proceed.Aug 23 2018, 11:18 PM
kib accepted this revision.Aug 24 2018, 6:24 PM
This revision is now accepted and ready to land.Aug 24 2018, 6:24 PM
This comment was removed by darrick.freebsd_gmail.com.
This revision now requires review to proceed.Oct 3 2018, 8:13 PM

Remove change to separate fix accidentally introduced.

kib accepted this revision.Oct 4 2018, 11:10 AM
This revision is now accepted and ready to land.Oct 4 2018, 11:10 AM