stack space reduction (remove MAX_BUF_PER_PAGE)
Subsystems (2)
PATCH 4/19 stack space reduction (remove MAX_BUF_PER_PAGE)
This patch fixes some potential stack consumption problems.
The storage for
sector_t blocks[MAX_BUF_PER_PAGE]; and struct buffer_head *arr[MAX_BUF_PER_PAGE];
will consume a kilobyte with 64k page, 64-bit sector_t. And on the path
do_mpage_readpage ->block_read_full_page -> <page allocation> ->mpage_writepage
they can be nested three-deep. 3k of stack gone. Presumably in this case the stack page would be 64k anyway, so that's not a problem. However if PAGE_SIZE=4k and PAGE_CACHE_SIZE=64k, we die.
I've yet to see any reason for larger PAGE_CACHE_SIZE, but this is a neater implementation anyway.
The patch removes MAX_BUF_PER_PAGE and instead walks the page's buffer ring to work out which buffers need to be submitted.
fs/mpage.c renamed from fs/mpage.c~stack-space +34 -23
@@ -169,8 +169,9 @@ do_mpage_readpage(struct bio *bio, strucconst unsigned blocksize =1 << blkbits;struct bio_vec *bvec;sector_t block_in_file; -sector_t last_block; -sector_t blocks[MAX_BUF_PER_PAGE ]; +sector_t last_file_block; +sector_t first_page_block =-1 ; +sector_t last_page_block =-1 ;unsigned page_block;unsigned first_hole = blocks_per_page;struct block_device *bdev =NULL ;
@@ -180,12 +181,12 @@ do_mpage_readpage(struct bio *bio, struc goto confused; block_in_file = page->index << (PAGE_CACHE_SHIFT - blkbits); - last_block = (inode->i_size + blocksize -1 ) >> blkbits; + last_file_block = (inode->i_size + blocksize -1 ) >> blkbits;for (page_block =0 ; page_block < blocks_per_page; page_block++, block_in_file++) { bh.b_state =0 ; -if (block_in_file < last_block) { +if (block_in_file < last_file_block) {if (get_block (inode, block_in_file, &bh,0 )) goto confused; }
@@ -199,10 +200,14 @@ do_mpage_readpage(struct bio *bio, strucif (first_hole != blocks_per_page) goto confused;/* hole -> non-hole */ -/* Contiguous blocks? */ -if (page_block && blocks[page_block-1 ] != bh.b_blocknr-1 ) - goto confused; - blocks[page_block] = bh.b_blocknr; +if (page_block) { +/* Contiguous blocks? */ +if (bh.b_blocknr != last_page_block +1 ) + goto confused; + }else { + first_page_block = bh.b_blocknr; + } + last_page_block = bh.b_blocknr; bdev = bh.b_bdev; }
@@ -222,7 +227,7 @@ do_mpage_readpage(struct bio *bio, struc* This page will go to BIO. Do we need to send this BIO off first? */ if (bio && (bio->bi_idx == bio->bi_vcnt || - *last_block_in_bio != blocks[0 ] -1 )) + *last_block_in_bio != first_page_block -1 )) bio =mpage_bio_submit (READ , bio);if (bio ==NULL ) {
@@ -230,7 +235,7 @@ do_mpage_readpage(struct bio *bio, strucif (nr_bvecs > nr_pages) nr_bvecs = nr_pages; - bio =mpage_alloc (bdev, blocks[0 ] << (blkbits -9 ), + bio =mpage_alloc (bdev, first_page_block << (blkbits -9 ), nr_bvecs,GFP_KERNEL );if (bio ==NULL ) goto confused;
@@ -244,7 +249,7 @@ do_mpage_readpage(struct bio *bio, strucif (buffer_boundary (&bh) || (first_hole != blocks_per_page)) bio =mpage_bio_submit (READ , bio);else - *last_block_in_bio = blocks[blocks_per_page -1 ]; + *last_block_in_bio = last_page_block; out:return bio;
@@ -322,9 +327,10 @@ mpage_writepage(struct bio *bio, structunsigned long end_index;const unsigned blocks_per_page =PAGE_CACHE_SIZE >> blkbits;struct bio_vec *bvec; -sector_t last_block; +sector_t last_file_block;sector_t block_in_file; -sector_t blocks[MAX_BUF_PER_PAGE ]; +sector_t first_page_block =-1 ; +sector_t last_page_block =-1 ;unsigned page_block;unsigned first_unmapped = blocks_per_page;struct block_device *bdev =NULL ;
@@ -355,11 +361,13 @@ mpage_writepage(struct bio *bio, structif (!buffer_dirty (bh) || !buffer_uptodate (bh)) goto confused; -if (page_block) { -if (bh->b_blocknr != blocks[page_block-1 ] +1 ) +if (page_block++) { +if (bh->b_blocknr != last_page_block +1 ) goto confused; + }else { + first_page_block = bh->b_blocknr; } - blocks[page_block++] = bh->b_blocknr; + last_page_block = bh->b_blocknr; boundary =buffer_boundary (bh); bdev = bh->b_bdev; }while ((bh = bh->b_this_page) != head);
@@ -381,7 +389,7 @@ mpage_writepage(struct bio *bio, struct*/ BUG_ON (!PageUptodate (page)); block_in_file = page->index << (PAGE_CACHE_SHIFT - blkbits); - last_block = (inode->i_size -1 ) >> blkbits; + last_file_block = (inode->i_size -1 ) >> blkbits;for (page_block =0 ; page_block < blocks_per_page; ) {struct buffer_head map_bh;
@@ -392,13 +400,16 @@ mpage_writepage(struct bio *bio, structunmap_underlying_metadata (map_bh.b_bdev, map_bh.b_blocknr);if (page_block) { -if (map_bh.b_blocknr != blocks[page_block-1 ] +1 ) +if (map_bh.b_blocknr != last_page_block +1 ) goto confused; + }else { + first_page_block = map_bh.b_blocknr; } - blocks[page_block++] = map_bh.b_blocknr; + page_block++; + last_page_block = map_bh.b_blocknr; boundary =buffer_boundary (&map_bh); bdev = map_bh.b_bdev; -if (block_in_file == last_block) +if (block_in_file == last_file_block)break ; block_in_file++; }
@@ -424,13 +435,13 @@ page_is_mapped:* This page will go to BIO. Do we need to send this BIO off first? */ if (bio && (bio->bi_idx == bio->bi_vcnt || - *last_block_in_bio != blocks[0 ] -1 )) + *last_block_in_bio != first_page_block -1 )) bio =mpage_bio_submit (WRITE , bio);if (bio ==NULL ) {unsigned nr_bvecs =MPAGE_BIO_MAX_SIZE /PAGE_CACHE_SIZE ; - bio =mpage_alloc (bdev, blocks[0 ] << (blkbits -9 ), + bio =mpage_alloc (bdev, first_page_block << (blkbits -9 ), nr_bvecs,GFP_NOFS );if (bio ==NULL ) goto confused;
@@ -464,7 +475,7 @@ page_is_mapped:if (boundary || (first_unmapped != blocks_per_page)) bio =mpage_bio_submit (WRITE , bio);else - *last_block_in_bio = blocks[blocks_per_page -1 ]; + *last_block_in_bio = last_page_block; goto out; confused:
fs/buffer.c renamed from fs/buffer.c~stack-space +19 -17
@@ -1799,7 +1799,7 @@ int block_read_full_page(struct page *pa {struct inode *inode = page->mapping->host;unsigned long iblock, lblock; -struct buffer_head *bh, *head, *arr[MAX_BUF_PER_PAGE ]; +struct buffer_head *bh, *head;unsigned int blocksize, blocks;int nr, i;
@@ -1842,7 +1842,7 @@ int block_read_full_page(struct page *paif (buffer_uptodate (bh))continue ; } - arr[nr++] = bh; + nr++; }while (i++, iblock++, (bh = bh->b_this_page) != head);if (!nr) {
@@ -1857,24 +1857,26 @@ int block_read_full_page(struct page *pa }/* Stage two: lock the buffers */ -for (i =0 ; i < nr; i++) { - bh = arr[i]; -lock_buffer (bh); -mark_buffer_async_read (bh); - } +do { +if (!buffer_uptodate (bh)) { +lock_buffer (bh); +mark_buffer_async_read (bh); + } + }while ((bh = bh->b_this_page) != head);/* * Stage 3: start the IO. Check for uptodateness * inside the buffer lock in case another process reading * the underlying blockdev brought it uptodate (the sct fix). */ -for (i =0 ; i < nr; i++) { - bh = arr[i]; -if (buffer_uptodate (bh)) -end_buffer_async_read (bh,1 ); -else -submit_bh (READ , bh); - } +do { +if (buffer_async_read (bh)) { +if (buffer_uptodate (bh)) +end_buffer_async_read (bh,1 ); +else +submit_bh (READ , bh); + } + }while ((bh = bh->b_this_page) != head);return 0 ; }
@@ -2490,8 +2492,8 @@ static void bh_mempool_free(void *elemenreturn kmem_cache_free (bh_cachep, element); } -#define NR_RESERVED (10*MAX_BUF_PER_PAGE) -#define MAX_UNUSED_BUFFERS NR_RESERVED+20 + +#define MEMPOOL_BUFFERS (32 * PAGE_CACHE_SIZE / 512)void __init buffer_init (void ) {
@@ -2500,7 +2502,7 @@ void __init buffer_init(void) bh_cachep =kmem_cache_create ("buffer_head" ,sizeof (struct buffer_head ),0 ,SLAB_HWCACHE_ALIGN , init_buffer_head,NULL ); - bh_mempool =mempool_create (MAX_UNUSED_BUFFERS , bh_mempool_alloc, + bh_mempool =mempool_create (MEMPOOL_BUFFERS , bh_mempool_alloc, bh_mempool_free,NULL );for (i =0 ; i <ARRAY_SIZE (bh_wait_queue_heads); i++)init_waitqueue_head (&bh_wait_queue_heads[i].wqh);
fs/block_dev.c renamed from fs/block_dev.c~stack-space +0 -2
@@ -23,8 +23,6 @@#include <asm/uaccess.h> -#define MAX_BUF_PER_PAGE (PAGE_CACHE_SIZE / 512) -static unsigned long max_block (struct block_device *bdev) {unsigned int retval = ~0U ;
include/linux/buffer_head.h renamed from include/linux/buffer_head.h~stack-space +0 -2
@@ -29,8 +29,6 @@ enum bh_state_bits {*/ }; -#define MAX_BUF_PER_PAGE (PAGE_CACHE_SIZE / 512) -struct page ;struct kiobuf ;struct buffer_head ;
fs/ntfs/aops.c renamed from fs/ntfs/aops.c~stack-space +63 -42
213 lines not on this page · raw message
-
Review
Quoted 6 lines · Andrew Morton
..
(quoted diff) +4
-0+ do { + if (buffer_async_read(bh)) + submit_bh(READ, bh); + } while ((bh = bh->b_this_page) != head);
That's a bug. We cannot touch bh->b_this_page after submitting the buffer because the I/O could complete synchronously and the page can come unlocked and the buffers can be stripped by the VM. The buffer at `bh' could be dead memory.
This can happen on SMP with PIO-mode IDE disks.
We cannot fix this with the usual
do {
next = bh->b_this_page;
if (something)
submit_bh(bh);
} while ((bh = next) != head);approach because the final buffer on the page could be unlocked, clean and uptodate.
Pinning one of the buffers while we walk the ring will keeptry_to_free_buffers() away. Here is an incremental patch.
fs/ntfs/aops.c renamed from fs/ntfs/aops.c~ntfs-race-fix +6 -0
@@ -220,10 +220,12 @@ handle_zblock: }while ((bh = bh->b_this_page) != head);/* Finally, start i/o on the buffers. */ +get_bh (head);/* Pin the buffers while we walk the ring */ do {if (buffer_async_read (bh))submit_bh (READ , bh); }while ((bh = bh->b_this_page) != head); +put_bh (head);return 0 ; }/* No i/o was scheduled on any of the buffers. */
@@ -510,10 +512,12 @@ handle_zblock: }while ((bh = bh->b_this_page) != head);/* Finally, start i/o on the buffers. */ +get_bh (head);/* Pin the buffers while we walk the ring */ do {if (buffer_async_read (bh))submit_bh (READ , bh); }while ((bh = bh->b_this_page) != head); +put_bh (head);return 0 ; }/* No i/o was scheduled on any of the buffers. */
@@ -774,10 +778,12 @@ handle_zblock: }while ((bh = bh->b_this_page) != head);/* Finally, start i/o on the buffers. */ +get_bh (head);/* Pin the buffers while we walk the ring */ do {if (buffer_async_read (bh))submit_bh (READ , bh); }while ((bh = bh->b_this_page) != head); +put_bh (head);return 0 ; }/* No i/o was scheduled on any of the buffers. */
fs/buffer.c renamed from fs/buffer.c~ntfs-race-fix +6 -0
@@ -2024,7 +2024,12 @@ int block_read_full_page(struct page *pa* Stage 3: start the IO. Check for uptodateness * inside the buffer lock in case another process reading * the underlying blockdev brought it uptodate (the sct fix). +* +* Bump the refcount of one buffer while walking the ring so +* that the VM cannot release the buffers while we're looking at +* ->b_this_page. */ +get_bh (head); do { if (buffer_async_read(bh)) { if (buffer_uptodate(bh))
@@ -2033,6 +2038,7 @@ int block_read_full_page(struct page *pasubmit_bh (READ , bh); } }while ((bh = bh->b_this_page) != head); +put_bh (head);return 0 ; }
-
Quoted 9 lines · Andrew Morton
Review
Quoted 6 lines · Andrew Morton
..
(quoted diff) +4
-0+ do { + if (buffer_async_read(bh)) + submit_bh(READ, bh); + } while ((bh = bh->b_this_page) != head);That's a bug.
I'm just not going to apply this patch.
Right now MAX_BUFFERS_PER_PAGE is 8 or 16, and the array of buffer heads is thus 32 bytes on an x86. I'd much rather get a nice tight loop without callbacks in the middle, and then submit 8 pre-approved buffers in one go, than have issues like this.
If we have 64kB pages, such architectures will have to have a bigger kernel stack. Which they will have, simply by virtue of having the very same bigger page. So that problem kind of solves itself.
Linus
Quoted 10 lines · Linus Torvalds
Quoted 8 lines · Andrew Morton
Quoted 6 lines · Andrew Morton
..
(quoted diff) +4
-0+ do { + if (buffer_async_read(bh)) + submit_bh(READ, bh); + } while ((bh = bh->b_this_page) != head);
Quoted 3 lines
Quoted 1 line
That's a bug.
I'm just not going to apply this patch.
Excellent idea.
There were some patches floating around a while back (Ben and Hugh, I think) which gave x86 a 64k soft PAGE_CACHE_SIZE. They will hit this problem. But they know about it now, and the fix is there.
-