ReiserFS CPU and memory bandwidth efficient large writes
PATCH 1/1 ReiserFS CPU and memory bandwidth efficient large writes
Hello!
These three changesets implement reiserfs_file_write. Also third one exports generic_osync_inode,block_commit_write and remove_suid because these are now needed for reiserfs.
There was no reiserfs_file_write in the 2.4 port of reiserfs (and Hans was very unhappy about it).
With current 'one block at a time' algorithm, writes past the end of a file are slow because each new file block is separately added into the tree causing shifting of other items which is CPU expensive.
With this new implementation if you write into file with big enough chunks, it uses half as much CPU. Also this version is more SMP friendly than the current one.
You can pull these from bk://thebsh.namesys.com/bk/reiser3-linux-2.5-own-filewrite
[email protected], 2002-12-07 16:56:59+03:00, [email protected]
export generic_osync_inode,block_commit_write, remove_suid
[email protected], 2002-12-07 16:55:03+03:00, [email protected]
reiserfs_file_write() implemenation. Ported from 2.4
[email protected], 2002-12-07 13:38:05+03:00, [email protected]
reiserfs: Implement insertion of more than one unformatted pointer insertion at a time. This considerably speedups hole creation.
Diffstat:
fs/reiserfs/bitmap.c | 21 fs/reiserfs/do_balan.c | 105 ++-- fs/reiserfs/file.c | 1071 ++++++++++++++++++++++++++++++++++++++++- fs/reiserfs/inode.c | 52 + fs/reiserfs/super.c | 1 fs/reiserfs/tail_conversion.c | 5 include/linux/reiserfs_fs.h | 1 include/linux/reiserfs_fs_sb.h | 1 kernel/ksyms.c | 3 9 files changed, 1193 insertions(+), 67 deletions(-)
Plain text patch:
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.844 -> 1.847 # fs/reiserfs/file.c 1.18 -> 1.19 # kernel/ksyms.c 1.169 -> 1.170
# fs/reiserfs/do_balan.c 1.15 -> 1.16
# fs/reiserfs/inode.c 1.68 -> 1.70
# fs/reiserfs/super.c 1.54 -> 1.55
# include/linux/reiserfs_fs_sb.h 1.20 -> 1.21
# fs/reiserfs/bitmap.c 1.24 -> 1.25
# include/linux/reiserfs_fs.h 1.45 -> 1.46
# fs/reiserfs/tail_conversion.c 1.23 -> 1.24
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/12/07 [email protected] 1.845
# reiserfs: Implement insertion of more than one unformatted pointer insertion at a time. This considerably speedups hole creation. # --------------------------------------------
# 02/12/07 [email protected] 1.846
# reiserfs_file_write() implemenation. Ported from 2.4
# --------------------------------------------
# 02/12/07 [email protected] 1.847
# export generic_osync_inode,block_commit_write, remove_suid # -------------------------------------------- # diff -Nru a/fs/reiserfs/bitmap.c b/fs/reiserfs/bitmap.c --- a/fs/reiserfs/bitmap.c Sat Dec 7 16:58:48 2002 +++ b/fs/reiserfs/bitmap.c Sat Dec 7 16:58:48 2002 @@ -9,6 +9,7 @@ #include <linux/errno.h> #include <linux/buffer_head.h> #include <linux/kernel.h> +#include <linux/pagemap.h> #include <linux/reiserfs_fs_sb.h> #include <linux/reiserfs_fs_i.h>
@@ -733,7 +734,7 @@
int rest = amount_needed;
int nr_allocated;- while (rest > 0) {
+ while (rest > 0 && start <= finish) {
nr_allocated = scan_bitmap (hint->th, &start, finish, 1,
rest + prealloc_size, !hint->formatted_node, hint->block);
@@ -879,7 +880,9 @@
if ( !blocks )
return;
+ spin_lock(&REISERFS_SB(sb)->bitmap_lock);
REISERFS_SB(sb)->reserved_blocks += blocks;+ spin_unlock(&REISERFS_SB(sb)->bitmap_lock);
}
/* Unreserve @blocks amount of blocks in fs pointed by @sb */
@@ -896,6 +899,22 @@
if ( !blocks )
return;
+ spin_lock(&REISERFS_SB(sb)->bitmap_lock);
REISERFS_SB(sb)->reserved_blocks -= blocks;
+ spin_unlock(&REISERFS_SB(sb)->bitmap_lock);
RFALSE( REISERFS_SB(sb)->reserved_blocks < 0, "amount of blocks reserved became zero?");+}
+
+/* This function estimates how much pages we will be able to write to FS
+ used for reiserfs_file_write() purposes for now. */
+int reiserfs_can_fit_pages ( struct super_block *sb /* superblock of filesystem
+ to estimate space */ )
+{
+ unsigned long space;
+
+ spin_lock(&REISERFS_SB(sb)->bitmap_lock);
+ space = (SB_FREE_BLOCKS(sb) - REISERFS_SB(sb)->reserved_blocks) / ( PAGE_CACHE_SIZE/sb->s_blocksize);
+ spin_unlock(&REISERFS_SB(sb)->bitmap_lock);
+
+ return space;
}
fs/reiserfs/do_balan.c +53 -52
@@ -319,8 +319,6 @@int new_item_len;int version; -RFALSE (!is_direct_le_ih (ih), -"PAP-12075: only direct inserted item can be broken. %h" , ih); ret_val =leaf_shift_left (tb, tb->lnum[0 ]-1 ,-1 );/* Calculate item length to insert to S[0] */
@@ -343,7 +341,7 @@ version =ih_version (ih);/* Calculate key component, item length and body to insert into S[0] */ -set_le_ih_k_offset ( ih,le_ih_k_offset ( ih ) + tb->lbytes ); +set_le_ih_k_offset ( ih,le_ih_k_offset ( ih ) + tb->lbytes * (is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize/UNFM_P_SIZE :1 ) );put_ih_item_len ( ih, new_item_len );if ( tb->lbytes > zeros_num ) {
@@ -452,23 +450,28 @@ih_item_len (B_N_PITEM_HEAD (tb->L[0 ],n+item_pos-ret_val)), l_n,body, zeros_num > l_n ? l_n : zeros_num ); - -RFALSE ( l_n && -is_indirect_le_ih (B_N_PITEM_HEAD - (tb->L[0 ], - n + item_pos - ret_val)), -"PAP-12110: pasting more than 1 unformatted node pointer into indirect item" ); -/* 0-th item in S0 can be only of DIRECT type when l_n != 0*/ { -int version; - - version =ih_version (B_N_PITEM_HEAD (tbS0,0 )); -set_le_key_k_offset (version,B_N_PKEY (tbS0,0 ), -le_key_k_offset (version,B_N_PKEY (tbS0,0 )) + l_n); - version =ih_version (B_N_PITEM_HEAD (tb->CFL[0 ],tb->lkey[0 ])); -set_le_key_k_offset (version,B_N_PDELIM_KEY (tb->CFL[0 ],tb->lkey[0 ]), -le_key_k_offset (version,B_N_PDELIM_KEY (tb->CFL[0 ],tb->lkey[0 ])) + l_n); +int version; +int temp_l = l_n; + +RFALSE (ih_item_len (B_N_PITEM_HEAD (tbS0,0 )), +"PAP-12106: item length must be 0" ); +RFALSE (comp_short_le_keys (B_N_PKEY (tbS0,0 ), +B_N_PKEY (tb->L[0 ], + n + item_pos - ret_val)), +"PAP-12107: items must be of the same file" ); +if (is_indirect_le_ih (B_N_PITEM_HEAD (tb->L[0 ], + n + item_pos - ret_val))) { + temp_l = (l_n /UNFM_P_SIZE ) * tb->tb_sb->s_blocksize; + } +/* update key of first item in S0 */ + version =ih_version (B_N_PITEM_HEAD (tbS0,0 )); +set_le_key_k_offset (version,B_N_PKEY (tbS0,0 ), +le_key_k_offset (version,B_N_PKEY (tbS0,0 )) + temp_l); +/* update left delimiting key */ +set_le_key_k_offset (version,B_N_PDELIM_KEY (tb->CFL[0 ],tb->lkey[0 ]), +le_key_k_offset (version,B_N_PDELIM_KEY (tb->CFL[0 ],tb->lkey[0 ])) + temp_l); }/* Calculate new body, position in item and insert_size[0] */
@@ -537,7 +540,7 @@ );/* if appended item is indirect item, put unformatted node into un list */ if (is_indirect_le_ih (pasted)) -set_ih_free_space (pasted, ((struct unfm_nodeinfo *)body)->unfm_freespace); +set_ih_free_space (pasted,0 ); tb->insert_size[0 ] =0 ; zeros_num =0 ; }
@@ -565,15 +568,11 @@ {/* new item or its part falls to R[0] */ if ( item_pos == n - tb->rnum[0 ] +1 && tb->rbytes !=-1 ) {/* part of new item falls into R[0] */ -int old_key_comp, old_len, r_zeros_number; +loff_t old_key_comp, old_len, r_zeros_number;const char * r_body;int version;loff_t offset; -RFALSE ( !is_direct_le_ih (ih), -"PAP-12135: only direct item can be split. (%h)" , - ih); -leaf_shift_right (tb,tb->rnum[0 ]-1 ,-1 ); version =ih_version (ih);
@@ -582,7 +581,7 @@ old_len =ih_item_len (ih);/* Calculate key component and item length to insert into R[0] */ - offset =le_ih_k_offset ( ih ) + (old_len - tb->rbytes ); + offset =le_ih_k_offset ( ih ) + (old_len - tb->rbytes )*(is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize/UNFM_P_SIZE :1 );set_le_ih_k_offset ( ih, offset );put_ih_item_len ( ih, tb->rbytes);/* Insert part of the item into R[0] */
@@ -590,13 +589,13 @@ bi.bi_bh = tb->R[0 ]; bi.bi_parent = tb->FR[0 ]; bi.bi_position =get_right_neighbor_position (tb,0 ); -if ( offset - old_key_comp > zeros_num ) { +if ( (old_len - tb->rbytes) > zeros_num ) { r_zeros_number =0 ; - r_body = body + offset - old_key_comp - zeros_num; + r_body = body + (old_len - tb->rbytes) - zeros_num; }else { r_body = body; - r_zeros_number = zeros_num - (offset - old_key_comp); + r_zeros_number = zeros_num - (old_len - tb->rbytes); zeros_num -= r_zeros_number; }
@@ -707,12 +706,17 @@ {int version; +unsigned long temp_rem = n_rem; version =ih_version (B_N_PITEM_HEAD (tb->R[0 ],0 )); +if (is_indirect_le_key (version,B_N_PKEY (tb->R[0 ],0 ))){ + temp_rem = (n_rem /UNFM_P_SIZE ) * + tb->tb_sb->s_blocksize; + }set_le_key_k_offset (version,B_N_PKEY (tb->R[0 ],0 ), -le_key_k_offset (version,B_N_PKEY (tb->R[0 ],0 )) + n_rem); +le_key_k_offset (version,B_N_PKEY (tb->R[0 ],0 )) + temp_rem);set_le_key_k_offset (version,B_N_PDELIM_KEY (tb->CFR[0 ],tb->rkey[0 ]), -le_key_k_offset (version,B_N_PDELIM_KEY (tb->CFR[0 ],tb->rkey[0 ])) + n_rem); +le_key_k_offset (version,B_N_PDELIM_KEY (tb->CFR[0 ],tb->rkey[0 ])) + temp_rem); }/* k_offset (B_N_PKEY(tb->R[0],0)) += n_rem; k_offset (B_N_PDELIM_KEY(tb->CFR[0],tb->rkey[0])) += n_rem;*/
@@ -736,13 +740,12 @@leaf_paste_in_buffer (&bi,0 , n_shift, tb->insert_size[0 ] - n_rem, r_body, r_zeros_number);if (is_indirect_le_ih (B_N_PITEM_HEAD (tb->R[0 ],0 ))) { - +#if 0 RFALSE ( n_rem,"PAP-12160: paste more than one unformatted node pointer" ); - -set_ih_free_space (B_N_PITEM_HEAD (tb->R[0 ],0 ), ((struct unfm_nodeinfo *)body)->unfm_freespace); +#endif +set_ih_free_space (B_N_PITEM_HEAD (tb->R[0 ],0 ),0 ); } - tb->insert_size[0 ] = n_rem;if ( ! n_rem ) pos_in_item ++;
@@ -781,7 +784,7 @@ }if (is_indirect_le_ih (pasted)) -set_ih_free_space (pasted, ((struct unfm_nodeinfo *)body)->unfm_freespace); +set_ih_free_space (pasted,0 ); zeros_num = tb->insert_size[0 ] =0 ; } }
@@ -858,12 +861,6 @@const char * r_body;int version; -RFALSE ( !is_direct_le_ih (ih), -/* The items which can be inserted are: -Stat_data item, direct item, indirect item and directory item which consist of only two entries "." and "..". -These items must not be broken except for a direct one. */ -"PAP-12205: non-direct item can not be broken when inserting" ); -/* Move snum[i]-1 items from S[0] to S_new[i] */ leaf_move_items (LEAF_FROM_S_TO_SNEW , tb, snum[i] -1 ,-1 , S_new[i]);/* Remember key component and item length */
@@ -873,7 +870,7 @@/* Calculate key component and item length to insert into S_new[i] */ set_le_ih_k_offset ( ih, -le_ih_k_offset (ih) + (old_len - sbytes[i] ) ); +le_ih_k_offset (ih) + (old_len - sbytes[i] )*(is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize/UNFM_P_SIZE :1 ) );put_ih_item_len ( ih, sbytes[i] );
@@ -883,13 +880,13 @@ bi.bi_parent =0 ; bi.bi_position =0 ; -if (le_ih_k_offset (ih) - old_key_comp > zeros_num ) { +if ( (old_len - sbytes[i]) > zeros_num ) { r_zeros_number =0 ; - r_body = body + (le_ih_k_offset (ih) - old_key_comp) - zeros_num; + r_body = body + (old_len - sbytes[i]) - zeros_num; }else { r_body = body; - r_zeros_number = zeros_num - (le_ih_k_offset (ih) - old_key_comp); + r_zeros_number = zeros_num - (old_len - sbytes[i]); zeros_num -= r_zeros_number; }
@@ -1010,11 +1007,14 @@ tmp =B_N_PITEM_HEAD (S_new[i],0 );if (is_indirect_le_ih (tmp)) { -if (n_rem) -reiserfs_panic (tb->tb_sb,"PAP-12230: balance_leaf: invalid action with indirect item" ); -set_ih_free_space (tmp, ((struct unfm_nodeinfo *)body)->unfm_freespace); +set_ih_free_space (tmp,0 ); +set_le_ih_k_offset ( tmp,le_ih_k_offset (tmp) + + (n_rem /UNFM_P_SIZE ) * + tb->tb_sb->s_blocksize); + }else { +set_le_ih_k_offset ( tmp,le_ih_k_offset (tmp) + + n_rem ); } -set_le_ih_k_offset ( tmp,le_ih_k_offset (tmp) + n_rem ); } tb->insert_size[0 ] = n_rem;
@@ -1060,7 +1060,7 @@/* if we paste to indirect item update ih_free_space */ if (is_indirect_le_ih (pasted)) -set_ih_free_space (pasted, ((struct unfm_nodeinfo *)body)->unfm_freespace); +set_ih_free_space (pasted,0 ); zeros_num = tb->insert_size[0 ] =0 ; } }
@@ -1152,11 +1152,12 @@leaf_paste_in_buffer (&bi, item_pos, pos_in_item, tb->insert_size[0 ], body, zeros_num);if (is_indirect_le_ih (pasted)) { - +#if 0 RFALSE ( tb->insert_size[0 ] !=UNFM_P_SIZE ,"PAP-12280: insert_size for indirect item must be %d, not %d" ,UNFM_P_SIZE , tb->insert_size[0 ]); -set_ih_free_space (pasted, ((struct unfm_nodeinfo *)body)->unfm_freespace); +#endif +set_ih_free_space (pasted,0 ); } tb->insert_size[0 ] =0 ; }
fs/reiserfs/file.c +1070 -1
1,085 lines not on this page · raw message
fs/reiserfs/inode.c +42 -10
93 lines not on this page · raw message
fs/reiserfs/super.c +1 -0
@@ -1313,6 +1313,7 @@reiserfs_proc_register ( s,"oidmap" , reiserfs_oidmap_in_proc );reiserfs_proc_register ( s,"journal" , reiserfs_journal_in_proc );init_waitqueue_head (&(sbi->s_wait)); + sbi->bitmap_lock =SPIN_LOCK_UNLOCKED ;return (0 );
fs/reiserfs/tail_conversion.c +2 -3
@@ -30,7 +30,7 @@key of unfm pointer to be pasted */ int n_blk_size, n_retval;/* returned value for reiserfs_insert_item and clones */ -struct unfm_nodeinfo unfm_ptr;/* Handle on an unformatted node +unp_t unfm_ptr;/* Handle on an unformatted node that will be inserted in the tree. */
@@ -59,8 +59,7 @@ p_le_ih =PATH_PITEM_HEAD (path); - unfm_ptr.unfm_nodenum =cpu_to_le32 (unbh->b_blocknr); - unfm_ptr.unfm_freespace =0 ;// ??? + unfm_ptr =cpu_to_le32 (unbh->b_blocknr);if (is_statdata_le_ih (p_le_ih) ) {/* Insert new indirect item. */
include/linux/reiserfs_fs.h +1 -0
@@ -2110,6 +2110,7 @@#endif void reiserfs_claim_blocks_to_be_allocated (struct super_block *sb,int blocks);void reiserfs_release_claimed_blocks (struct super_block *sb,int blocks); +int reiserfs_can_fit_pages (struct super_block *sb);/* hashes.c */ __u32 keyed_hash (const signed char *msg,int len);
include/linux/reiserfs_fs_sb.h +1 -0
@@ -397,6 +397,7 @@reiserfs_proc_info_data_t s_proc_info_data;struct proc_dir_entry *procdir;int reserved_blocks;/* amount of blocks reserved for further allocations */ +spinlock_t bitmap_lock;/* this lock on now only used to protect reserved_blocks variable */ };/* Definitions of reiserfs on-disk properties: */
kernel/ksyms.c +3 -0
@@ -217,6 +217,7 @@EXPORT_SYMBOL (generic_cont_expand);EXPORT_SYMBOL (cont_prepare_write);EXPORT_SYMBOL (generic_commit_write); +EXPORT_SYMBOL (block_commit_write);EXPORT_SYMBOL (block_truncate_page);EXPORT_SYMBOL (generic_block_bmap);EXPORT_SYMBOL (generic_file_read);
@@ -544,6 +545,8 @@EXPORT_SYMBOL (make_bad_inode);EXPORT_SYMBOL (is_bad_inode);EXPORT_SYMBOL (__inode_dir_notify); +EXPORT_SYMBOL (generic_osync_inode); +EXPORT_SYMBOL (remove_suid);#ifdef CONFIG_UID16 EXPORT_SYMBOL (overflowuid);
Quoted 2 lines · Hans Reiser
..
These three changesets implementreiserfs_file_write.
What a big patch.
I'd be interested in testing it a bit. Could you please describe under what circumstances the new code paths are executed? Is it simply appending to a normal old file? No special mount options?
A few little comments, and maybe a security problem....
Review
Quoted 11 lines
(quoted diff) +10
-0+int reiserfs_can_fit_pages ( struct super_block *sb /* superblock of filesystem + to estimate space */ ) +{ + unsigned long space; + + spin_lock(&REISERFS_SB(sb)->bitmap_lock); + space = (SB_FREE_BLOCKS(sb) - REISERFS_SB(sb)->reserved_blocks) / ( PAGE_CACHE_SIZE/sb->s_blocksize); + spin_unlock(&REISERFS_SB(sb)->bitmap_lock); + + return space; }
Both of the divisions here can be replaced with shifts. Divides are expensive.
The locking here is peculiar:
spin_lock(lock); value = calculation(); spin_unlock(lock); return value;
Surely, if the calculation relies on the lock then the returned value is invalidated as soon as you drop the lock?
fs/reiserfs/do_balan.c line 584
Quoted 2 lines
fs/reiserfs/do_balan.c +1
-1- offset =le_ih_k_offset ( ih ) + (old_len - tb->rbytes ); + offset =le_ih_k_offset ( ih ) + (old_len - tb->rbytes )*(is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize/UNFM_P_SIZE :1 );
Can use a shift.
fs/reiserfs/do_balan.c line 714
Quoted 3 lines
fs/reiserfs/do_balan.c +3
-0+if (is_indirect_le_key (version,B_N_PKEY (tb->R[0 ],0 ))){ + temp_rem = (n_rem /UNFM_P_SIZE ) * + tb->tb_sb->s_blocksize;
Shift by i_blkbits (all over the place)
fs/reiserfs/file.c line 147
Quoted 3 lines
fs/reiserfs/file.c +3
-0+/* this function from inode.c would be used here, too */ +extern void restart_transaction (struct reiserfs_transaction_handle *th, +struct inode *inode,struct path *path);
This decl should be in a header file.
fs/reiserfs/file.c line 153
Quoted 6 lines
fs/reiserfs/file.c +6
-0+ +/* I really do not want to play with memory shortage right now, so +to simplify the code, we are not going to write more than this much pages at +a time. This still should considerably improve performance compared to 4k +at a time case. */ +#define REISERFS_WRITE_PAGES_AT_A_TIME 32
Page sizes vary. A better value may be
(128 * 1024) / PAGE_CACHE_SIZE
so that consistent behaviour is seem on platforms which have page sizes larger than 4k.
fs/reiserfs/file.c line 179
Quoted 3 lines
fs/reiserfs/file.c +2
-0+int reiserfs_allocate_blocks_for_region ( + b_blocknr_t allocated_blocks[blocks_to_allocate]; // Pointer to a place where allocated blocknumbers would be stored. Right now statically allocated, later that will change.
This is a variable-sized array (aka: alloca). It's not a problem IMO, but worth pointing out...
fs/reiserfs/file.c line 191
Quoted 12 lines
fs/reiserfs/file.c +12
-0+reiserfs_blocknr_hint_t hint;// hint structure for block allocator. +size_t res;// return value of various functions that we call. +int curr_block;// current block used to keep track of unmapped blocks. +int i;// loop counter +int itempos;// position in item +unsigned int from = (pos & (PAGE_CACHE_SIZE -1 ));// writing position in +// first page +unsigned int to = ((pos + write_bytes -1 ) & (PAGE_CACHE_SIZE -1 )) +1 ;/* last modified byte offset in last page */ +__u64 hole_size ;// amount of blocks for a file hole, if it needed to be created. +int modifying_this_item =0 ;// Flag for items traversal code to keep track +// of the fact that we already prepared +// current block for journal
How much stack is this function using, worst-case?
fs/reiserfs/file.c line 764
Quoted 2 lines
fs/reiserfs/file.c +2
-0+for ( i =0 ; i < num_pages; i++) { + prepared_pages[i] =grab_cache_page (mapping, index + i);// locks the page
OK. But note that locking multiple pages here is only free from AB/BA deadlocks because this is the only path which does it, and it is singly-threaded via i_sem.
fs/reiserfs/file.c line 808
Quoted 12 lines
fs/reiserfs/file.c +12
-0+if ( from !=0 ) {/* First page needs to be partially zeroed */ +char *kaddr =kmap_atomic (prepared_pages[0 ],KM_USER0 ); +memset (kaddr,0 , from); +kunmap_atomic ( kaddr,KM_USER0 ); +SetPageUptodate (prepared_pages[0 ]); + } +if ( to !=PAGE_CACHE_SIZE ) {/* Last page needs to be partially zeroed */ +char *kaddr =kmap_atomic (prepared_pages[num_pages-1 ],KM_USER0 ); +memset (kaddr+to,0 ,PAGE_CACHE_SIZE - to); +kunmap_atomic ( kaddr,KM_USER0 ); +SetPageUptodate (prepared_pages[num_pages-1 ]); + }
This seems wrong. This could be a newly-allocated pagecache page. It is not yet fully uptodate. If (say) the subsequent copy_from_user gets a fault then it appears that this now-uptodate pagecache page will leak uninitialised stuff?
Review
Quoted 1 line
(quoted diff) +1
-0+ set_bit(BH_Uptodate, &bh->b_state);
set_buffer_uptodate(bh) is more conventional (two instances).
fs/reiserfs/inode.c line 819
Quoted 4 lines
fs/reiserfs/inode.c +4
-0+/* We need to mark new file size in case this function will be +interrupted/aborted later on. And we may do this only for +holes. */ + inode->i_size += inode->i_sb->s_blocksize * blocks_needed;
64-bit multiply, should be a shift.
Andrew, thanks much for these helpful suggestions and detailed review. Oleg will reply in detail on Monday.
Hans
Hello!
Quoted 2 lines · Andrew Morton
Quoted 1 line
These three changesets implement
reiserfs_file_write.What a big patch.
Yup. Also it was around for half a year or something ;) Also actually it consists of two parts(changesets).
Quoted 4 lines
I'd be interested in testing it a bit. Could you please describe under what circumstances the new code paths are executed? Is it simply appending to a normal old file? No special mount options?
Well, there are several paths all related to appending.
One path is to seek far beyond end of file and then write some amount of data (or just do a truncate beyond end of file) therefore creating a hole (with different code than before).
Also Just appending to a file in chunks bigger than 4k.
Somewhat different code paths if you append to just created file, file that is smaller than 4k in size and file that is bigger than 4k in size.
Quoted 1 line
A few little comments, and maybe a security problem....
Hopefuly not a problem.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+int reiserfs_can_fit_pages ( struct super_block *sb /* superblock of filesystem + to estimate space */ ) +{ + unsigned long space;
Quoted 8 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ + spin_lock(&REISERFS_SB(sb)->bitmap_lock); + space = (SB_FREE_BLOCKS(sb) - REISERFS_SB(sb)->reserved_blocks) / ( PAGE_CACHE_SIZE/sb->s_blocksize); + spin_unlock(&REISERFS_SB(sb)->bitmap_lock); + + return space; }Both of the divisions here can be replaced with shifts. Divides are expensive.
Well, this can be done of course.
Quoted 6 lines
The locking here is peculiar:
spin_lock(lock); value = calculation(); spin_unlock(lock); return value;
The locking is here for different reason. We protect against somebody changing REISERFS_SB(sb)->reserved_blocks value while we are calculating. If you'd look into where we I call reiserfs_can_fit_pages() in file.c, you'd notice first I am taking BKL. No other codepath increases REISERFS_SB(sb)->reserved_blocks value (we only increase it in the same region), also all "old style" allocations are done under BKL as well.
There is only possible decrease of REISERFS_SB(sb)->reserved_blocks that is not under BKL and is the whole reason I need this additional lock. Obviously decreasing REISERFS_SB(sb)->reserved_blocks is completely ok to happen.
Quoted 2 lines
Surely, if the calculation relies on the lock then the returned value is invalidated as soon as you drop the lock?
Well, kind of, but we only care if the calcutaion may become less and this cannot happen.
Quoted 4 lines
fs/reiserfs/file.c line 147
Quoted 3 lines
fs/reiserfs/file.c +3
-0+/* this function from inode.c would be used here, too */ +extern void restart_transaction (struct reiserfs_transaction_handle *th, +struct inode *inode,struct path *path);This decl should be in a header file.
Sure.
Quoted 9 lines
fs/reiserfs/file.c line 153
Quoted 5 lines
fs/reiserfs/file.c +5
-0+/* I really do not want to play with memory shortage right now, so +to simplify the code, we are not going to write more than this much pages at +a time. This still should considerably improve performance compared to 4k +at a time case. */ +#define REISERFS_WRITE_PAGES_AT_A_TIME 32Page sizes vary. A better value may be
(128 * 1024) / PAGE_CACHE_SIZEso that consistent behaviour is seem on platforms which have page sizes larger than 4k.
Ok.
Quoted 5 lines
fs/reiserfs/file.c line 179
Quoted 3 lines
fs/reiserfs/file.c +2
-0+int reiserfs_allocate_blocks_for_region ( + b_blocknr_t allocated_blocks[blocks_to_allocate]; // Pointer to a place where allocated blocknumbers would be stored. Right now statically allocated, later that will change.This is a variable-sized array (aka: alloca). It's not a problem IMO, but worth pointing out...
Sure.
Quoted 5 lines
Quoted 5 lines
fs/reiserfs/file.c +5
-0+reiserfs_blocknr_hint_t hint;// hint structure for block allocator. +size_t res;// return value of various functions that we call. +int curr_block;// current block used to keep track of unmapped blocks. +int i;// loop counter +int itempos;// position in item
Quoted 8 lines
fs/reiserfs/file.c line 191
Quoted 7 lines
fs/reiserfs/file.c +7
-0+unsigned int from = (pos & (PAGE_CACHE_SIZE -1 ));// writing position in +// first page +unsigned int to = ((pos + write_bytes -1 ) & (PAGE_CACHE_SIZE -1 )) +1 ;/* last modified byte offset in last page */ +__u64 hole_size ;// amount of blocks for a file hole, if it needed to be created. +int modifying_this_item =0 ;// Flag for items traversal code to keep track +// of the fact that we already prepared +// current block for journal How much stack is this function using, worst-case?
For 4k blocksize, 4k pages, 32bit arch it will take at most:sizeof(struct cpu_key)+sizeof(void *)*3+sizeof(struct reiserfs_transaction_handle)+sizeof(struct path)+sizeof(unsigned int)*32+sizeof(reiserfs_blocknr_hint_t)+sizeof(size_t)+sizeof(int)*6+sizeof(__u64) = 340 bytes (only local variables obviously).
For 1k blocksize, 4k pages, 32bit arch it will take at most:sizeof(struct cpu_key)+sizeof(void *)*3+sizeof(struct reiserfs_transaction_handle)+sizeof(struct path)+sizeof(unsigned int)*32*4+sizeof(reiserfs_blocknr_hint_t)+sizeof(size_t)+sizeof(int)*6+sizeof(__u64) = 724 bytes (only local variables)
Quoted 4 lines
fs/reiserfs/file.c line 764
Quoted 2 lines
fs/reiserfs/file.c +2
-0+for ( i =0 ; i < num_pages; i++) { + prepared_pages[i] =grab_cache_page (mapping, index + i);// locks the page OK. But note that locking multiple pages here is only free from AB/BA deadlocks because this is the only path which does it, and it is singly-threaded via
i_sem.
It just a memo to not forget to release a page later. Also parallel mmap writer won't be able to touch these pages if these are yet unallocated.
Quoted 7 lines
Quoted 7 lines
fs/reiserfs/file.c +7
-0+if ( from !=0 ) {/* First page needs to be partially zeroed */ +char *kaddr =kmap_atomic (prepared_pages[0 ],KM_USER0 ); +memset (kaddr,0 , from); +kunmap_atomic ( kaddr,KM_USER0 ); +SetPageUptodate (prepared_pages[0 ]); + } +if ( to !=PAGE_CACHE_SIZE ) {/* Last page needs to be partially zeroed */
Quoted 8 lines
fs/reiserfs/file.c line 808
Quoted 5 lines
fs/reiserfs/file.c +5
-0+char *kaddr =kmap_atomic (prepared_pages[num_pages-1 ],KM_USER0 ); +memset (kaddr+to,0 ,PAGE_CACHE_SIZE - to); +kunmap_atomic ( kaddr,KM_USER0 ); +SetPageUptodate (prepared_pages[num_pages-1 ]); + }This seems wrong. This could be a newly-allocated pagecache page. It is not yet fully uptodate. If (say) the subsequent
copy_from_usergets a fault then it appears that this now-uptodate pagecache page will leak uninitialised stuff?
No, I do not see it. Even if we have somebody already mmapped this part of file, and he got enough of luck that subsequent copy_from_user gets a fault and then this someone gets to CPU and tries to access the page, the SIGBUS should happen because of access to mmaped area beyond end of file as we have not yet updated the file size note that we have this check before this code you pointed out:
if ( (pos & ~(PAGE_CACHE_SIZE - 1)) > inode->i_size ) {Or am I missing something?
Ok, actually I see how I can move this check to where I do copy_from_user.
I will take care of replacing multiplys and divisions with shifting. Thanks a lot for your review.
Bye,
Oleg
Quoted 17 lines · Oleg Drokin
Quoted 13 lines
fs/reiserfs/file.c line 808
Quoted 13 lines
fs/reiserfs/file.c +12
-0+if ( from !=0 ) {/* First page needs to be partially zeroed */ +char *kaddr =kmap_atomic (prepared_pages[0 ],KM_USER0 ); +memset (kaddr,0 , from); +kunmap_atomic ( kaddr,KM_USER0 ); +SetPageUptodate (prepared_pages[0 ]); + } +if ( to !=PAGE_CACHE_SIZE ) {/* Last page needs to be partially zeroed */ +char *kaddr =kmap_atomic (prepared_pages[num_pages-1 ],KM_USER0 ); +memset (kaddr+to,0 ,PAGE_CACHE_SIZE - to); +kunmap_atomic ( kaddr,KM_USER0 ); +SetPageUptodate (prepared_pages[num_pages-1 ]); + }Quoted 3 lines
This seems wrong. This could be a newly-allocated pagecache page. It is not yet fully uptodate. If (say) the subsequent
copy_from_usergets a fault then it appears that this now-uptodate pagecache page will leak uninitialised stuff?
Quoted 7 lines
No, I do not see it. Even if we have somebody already mmapped this part of file, and he got enough of luck that subsequent
copy_from_usergets a fault and then this someone gets to CPU and tries to access the page, the SIGBUS should happen because of access to mmaped area beyond end of file as we have not yet updated the file size note that we have this check before this code you pointed out:if ( (pos & ~(PAGE_CACHE_SIZE - 1)) > inode->i_size ) {
It is not related to mmap. The exploit would be to pass a partially (or fully?) invalid (address, length) pair into the write() system call.
Something like:
fd = creat(...); write(fd, 0, 4095); /* efault, instantiate 0'th page */ lseek(fd, 4096, SEEK_SET); write(fd, "", 1); /* place the 0'th page inside i_size */ lseek(fd, 0, SEEK_SET); read(fd, my_buffer, 4095); /* now what do we have? */
Hello!
Quoted 3 lines · Andrew Morton
This seems wrong. This could be a newly-allocated pagecache page. It is not yet fully uptodate. If (say) the subsequent
copy_from_usergets a fault then it appears that this now-uptodate pagecache page will leak uninitialised stuff?
Ok, after all I think we do not need this uptodate stuff at all.
Find below the patch that address all the issues you've brought. It is on top of previous one.
Do you think it is ok now?
Bye,
Oleg
===== fs/reiserfs/bitmap.c 1.25 vs edited =====
fs/reiserfs/bitmap.c +1 -1
@@ -913,7 +913,7 @@unsigned long space;spin_lock (&REISERFS_SB (sb)->bitmap_lock); - space = (SB_FREE_BLOCKS (sb) -REISERFS_SB (sb)->reserved_blocks) / (PAGE_CACHE_SIZE /sb->s_blocksize); + space = (SB_FREE_BLOCKS (sb) -REISERFS_SB (sb)->reserved_blocks) >> (PAGE_CACHE_SHIFT - sb->s_blocksize_bits);spin_unlock (&REISERFS_SB (sb)->bitmap_lock);return space;
===== fs/reiserfs/do_balan.c 1.16 vs edited =====
fs/reiserfs/do_balan.c +7 -8
@@ -341,7 +341,7 @@ version =ih_version (ih);/* Calculate key component, item length and body to insert into S[0] */ -set_le_ih_k_offset ( ih,le_ih_k_offset ( ih ) + tb->lbytes * (is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize/UNFM_P_SIZE :1 ) ); +set_le_ih_k_offset ( ih,le_ih_k_offset ( ih ) + (tb->lbytes >> (is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize_bits -UNFM_P_SHIFT :0 )) );put_ih_item_len ( ih, new_item_len );if ( tb->lbytes > zeros_num ) {
@@ -463,7 +463,7 @@ "PAP-12107: items must be of the same file");if (is_indirect_le_ih (B_N_PITEM_HEAD (tb->L[0 ], n + item_pos - ret_val))) { - temp_l = (l_n /UNFM_P_SIZE ) * tb->tb_sb->s_blocksize; + temp_l = (l_n /UNFM_P_SIZE ) << tb->tb_sb->s_blocksize_bits; }/* update key of first item in S0 */ version =ih_version (B_N_PITEM_HEAD (tbS0,0 ));
@@ -581,7 +581,7 @@ old_len =ih_item_len (ih);/* Calculate key component and item length to insert into R[0] */ - offset =le_ih_k_offset ( ih ) + (old_len - tb->rbytes )*(is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize/UNFM_P_SIZE :1 ); + offset =le_ih_k_offset ( ih ) + ((old_len - tb->rbytes )>>(is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize_bits -UNFM_P_SHIFT :0 ));set_le_ih_k_offset ( ih, offset );put_ih_item_len ( ih, tb->rbytes);/* Insert part of the item into R[0] */
@@ -710,8 +710,8 @@ version =ih_version (B_N_PITEM_HEAD (tb->R[0 ],0 ));if (is_indirect_le_key (version,B_N_PKEY (tb->R[0 ],0 ))){ - temp_rem = (n_rem /UNFM_P_SIZE ) * - tb->tb_sb->s_blocksize; + temp_rem = n_rem >> (tb->tb_sb->s_blocksize - +UNFM_P_SHIFT ); }set_le_key_k_offset (version,B_N_PKEY (tb->R[0 ],0 ),le_key_k_offset (version,B_N_PKEY (tb->R[0 ],0 )) + temp_rem);
@@ -870,7 +870,7 @@/* Calculate key component and item length to insert into S_new[i] */ set_le_ih_k_offset ( ih, -le_ih_k_offset (ih) + (old_len - sbytes[i] )*(is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize/UNFM_P_SIZE :1 ) ); +le_ih_k_offset (ih) + ((old_len - sbytes[i] )>>(is_indirect_le_ih (ih)?tb->tb_sb->s_blocksize_bits -UNFM_P_SHIFT :0 )) );put_ih_item_len ( ih, sbytes[i] );
@@ -1009,8 +1009,7 @@if (is_indirect_le_ih (tmp)) {set_ih_free_space (tmp,0 );set_le_ih_k_offset ( tmp,le_ih_k_offset (tmp) + - (n_rem /UNFM_P_SIZE ) * - tb->tb_sb->s_blocksize); + (n_rem >> (tb->tb_sb->s_blocksize_bits -UNFM_P_SHIFT ))); }else {set_le_ih_k_offset ( tmp,le_ih_k_offset (tmp) + n_rem );
===== fs/reiserfs/file.c 1.19 vs edited =====
fs/reiserfs/file.c +15 -21
@@ -142,15 +142,11 @@return error ; } -/* this function from inode.c would be used here, too */ -extern void restart_transaction (struct reiserfs_transaction_handle *th, -struct inode *inode,struct path *path); -/* I really do not want to play with memory shortage right now, so to simplify the code, we are not going to write more than this much pages at a time. This still should considerably improve performance compared to 4k -at a time case. */ -#define REISERFS_WRITE_PAGES_AT_A_TIME 32 +at a time case. This is 32 pages of 4k size. */ +#define REISERFS_WRITE_PAGES_AT_A_TIME (128 * 1024) / PAGE_CACHE_SIZE/* Allocates blocks for a file to fulfil write request. Maps all unmapped but prepared pages from the list.
@@ -217,7 +213,7 @@ hint.inode = inode;// Inode is needed by block allocator too. hint.search_start =0 ;// We have no hint on where to search free blocks for block allocator. hint.key = key.on_disk_key;// on disk key of file. - hint.block = inode->i_blocks/(inode->i_sb->s_blocksize/512 );// Number of disk blocks this file occupies already. + hint.block = inode->i_blocks>>(inode->i_sb->s_blocksize_bits-9 );// Number of disk blocks this file occupies already. hint.formatted_node =0 ;// We are allocating blocks for unformatted node. hint.preallocate =0 ;// We do not do any preallocation for now.
@@ -346,7 +342,7 @@restart_transaction (&th, inode, &path);/* Well, need to recalculate path and stuff */ -set_cpu_key_k_offset ( &key,cpu_key_k_offset (&key) + to_paste * inode->i_sb->s_blocksize ); +set_cpu_key_k_offset ( &key,cpu_key_k_offset (&key) + (to_paste << inode->i_blkbits)); res =search_for_position_by_key (inode->i_sb, &key, &path);if ( res ==IO_ERROR ) { res = -EIO ;
@@ -492,7 +488,7 @@ }/* Amount of on-disk blocks used by file have changed, update it */ - inode->i_blocks += blocks_to_allocate * (inode->i_sb->s_blocksize /512 ); + inode->i_blocks += blocks_to_allocate << (inode->i_blkbits -9 );reiserfs_update_sd (&th, inode);// And update on-disk metadata // finish all journal stuff now, We are not going to play with metadata // anymore.
@@ -775,18 +771,18 @@if ( num_pages >2 )/* These are full-overwritten pages so we count all the blocks in these pages are counted as needed to be allocated */ - blocks = (PAGE_CACHE_SIZE /inode->i_sb->s_blocksize) * (num_pages -2 ); + blocks = (num_pages -2 ) << (PAGE_CACHE_SHIFT - inode->i_blkbits);/* count blocks needed for first page (possibly partially written) */ - blocks += ((PAGE_CACHE_SIZE - from) >> inode->i_sb->s_blocksize_bits) + + blocks += ((PAGE_CACHE_SIZE - from) >> inode->i_blkbits) + !!(from & (inode->i_sb->s_blocksize-1 ));/* roundup */ /* Now we account for last page. If last page == first page (we overwrite only one page), we substract all the blocks past the last writing position in a page out of already calculated number of blocks */ - blocks += (PAGE_CACHE_SIZE /inode->i_sb->s_blocksize) * (num_pages >1 ) - - ((PAGE_CACHE_SIZE - to) >> inode->i_sb->s_blocksize_bits); + blocks += ((num_pages >1 ) << (PAGE_CACHE_SHIFT -inode->i_blkbits)) - + ((PAGE_CACHE_SIZE - to) >> inode->i_blkbits);/* Note how we do not roundup here since partial blocks still should be allocated */
@@ -798,13 +794,11 @@char *kaddr =kmap_atomic (prepared_pages[0 ],KM_USER0 );memset (kaddr,0 , from);kunmap_atomic ( kaddr,KM_USER0 ); -SetPageUptodate (prepared_pages[0 ]); }if ( to !=PAGE_CACHE_SIZE ) {/* Last page needs to be partially zeroed */ char *kaddr =kmap_atomic (prepared_pages[num_pages-1 ],KM_USER0 );memset (kaddr+to,0 ,PAGE_CACHE_SIZE - to);kunmap_atomic ( kaddr,KM_USER0 ); -SetPageUptodate (prepared_pages[num_pages-1 ]); }/* Since all blocks are new - use already calculated value */
@@ -921,7 +915,7 @@char *kaddr =kmap_atomic (prepared_pages[0 ],KM_USER0 );memset (kaddr+block_start,0 , from-block_start);kunmap_atomic ( kaddr,KM_USER0 ); -set_bit (BH_Uptodate, &bh->b_state); +set_buffer_uptodate (bh); } } }
@@ -953,7 +947,7 @@char *kaddr =kmap_atomic (prepared_pages[num_pages-1 ],KM_USER0 );memset (kaddr+to,0 , block_end-to);kunmap_atomic ( kaddr,KM_USER0 ); -set_bit (BH_Uptodate, &bh->b_state); +set_buffer_uptodate (bh); } } }
@@ -1127,7 +1121,7 @@for this iteration. */ num_pages =min_t (int,REISERFS_WRITE_PAGES_AT_A_TIME ,reiserfs_can_fit_pages (inode->i_sb));/* Also we should not forget to set size in bytes accordingly */ - write_bytes = num_pages *PAGE_CACHE_SIZE - + write_bytes = (num_pages <<PAGE_CACHE_SHIFT ) - (pos & (PAGE_CACHE_SIZE -1 ));/* If position is not on the start of the page, we need
@@ -1138,7 +1132,7 @@/* reserve the blocks to be allocated later, so that later on we still have the space to write the blocks to */ -reiserfs_claim_blocks_to_be_allocated (inode->i_sb, num_pages * (PAGE_CACHE_SIZE /inode->i_sb->s_blocksize)); +reiserfs_claim_blocks_to_be_allocated (inode->i_sb, num_pages << (PAGE_CACHE_SHIFT - inode->i_blkbits));reiserfs_write_unlock (inode->i_sb);if ( !num_pages ) {/* If we do not have enough space even for */
@@ -1162,12 +1156,12 @@ blocks_to_allocate =reiserfs_prepare_file_region_for_write (inode, pos, num_pages, write_bytes, prepared_pages);if ( blocks_to_allocate <0 ) { res = blocks_to_allocate; -reiserfs_release_claimed_blocks (inode->i_sb, num_pages * (PAGE_CACHE_SIZE /inode->i_sb->s_blocksize)); +reiserfs_release_claimed_blocks (inode->i_sb, num_pages << (PAGE_CACHE_SHIFT - inode->i_blkbits));break ; }/* First we correct our estimate of how many blocks we need */ -reiserfs_release_claimed_blocks (inode->i_sb, num_pages * (PAGE_CACHE_SIZE >>inode->i_sb->s_blocksize_bits) - blocks_to_allocate ); +reiserfs_release_claimed_blocks (inode->i_sb, (num_pages << (PAGE_CACHE_SHIFT - inode->i_sb->s_blocksize_bits)) - blocks_to_allocate );if ( blocks_to_allocate >0 ) {/*We only allocate blocks if we need to*/ /* Fill in all the possible holes and append the file if needed */
===== fs/reiserfs/inode.c 1.70 vs edited =====
fs/reiserfs/inode.c +1 -1
@@ -816,7 +816,7 @@/* We need to mark new file size in case this function will be interrupted/aborted later on. And we may do this only for holes. */ - inode->i_size += inode->i_sb->s_blocksize * blocks_needed; + inode->i_size += blocks_needed << inode->i_blkbits; }//mark_tail_converted (inode); }
===== include/linux/reiserfs_fs.h 1.46 vs edited =====
include/linux/reiserfs_fs.h +2 -1
@@ -1268,6 +1268,7 @@/* Size of pointer to the unformatted node. */ #define UNFM_P_SIZE (sizeof(unp_t)) +#define UNFM_P_SHIFT 2// in in-core inode key is stored on le form #define INODE_PKEY (inode) ((struct key *)(REISERFS_I(inode)->i_key))
@@ -1838,7 +1839,7 @@void padd_item (char * item,int total_length,int length);/* inode.c */ - +void restart_transaction (struct reiserfs_transaction_handle *th,struct inode *inode,struct path *path);void reiserfs_read_locked_inode (struct inode * inode,struct reiserfs_iget_args *args) ;int reiserfs_find_actor (struct inode * inode,void *p) ;int reiserfs_init_locked_inode (struct inode * inode,void *p) ;
Quoted 10 lines · Oleg Drokin
Hello!
Quoted 3 lines · Andrew Morton
This seems wrong. This could be a newly-allocated pagecache page. It is not yet fully uptodate. If (say) the subsequent
copy_from_usergets a fault then it appears that this now-uptodate pagecache page will leak uninitialised stuff?Ok, after all I think we do not need this uptodate stuff at all.
Well that certainly simplifies things.
Quoted 3 lines
Find below the patch that address all the issues you've brought. It is on top of previous one.
Do you think it is ok now?
I addresses the things I noticed and raised, thanks. Except for the stack-space use. People are waving around 4k-stack patches, and we do need to be careful there.
Hello!
Quoted 6 lines · Andrew Morton
Quoted 3 lines
Find below the patch that address all the issues you've brought. It is on top of previous one.
Do you think it is ok now?I addresses the things I noticed and raised, thanks. Except for the stack-space use. People are waving around 4k-stack patches, and we do need to be careful there.
Well, 450 bytes is way below 4k (~7 times less if we'd take task struct into account) ;)
I can replace that on-stack array with kmalloc, but that probably would be a lot of overhead for no benefit.
What do you think is safe stack usage limit for a function? (and btw you have not even seen reiser4 stack usage ;) )
Bye,
Oleg
Quoted 7 lines · Oleg Drokin
Hello!
Quoted 3 lines · Andrew Morton
Quoted 3 lines
Find below the patch that address all the issues you've brought. It is on top of previous one.
Do you think it is ok now?
Quoted 8 lines
Quoted 3 lines
I addresses the things I noticed and raised, thanks. Except for the stack-space use. People are waving around 4k-stack patches, and we do need to be careful there.
Well, 450 bytes is way below 4k (~7 times less if we'd take task struct into account) ;)
I can replace that on-stack array with kmalloc, but that probably would be a lot of overhead for no benefit.
It would be a little overhead. kmalloc is damn quick, and remember that this data has to be copied from userspace and has go to disk
sometime. These things will make the kmalloc overhead very small.
Quoted 1 line
What do you think is safe stack usage limit for a function?
As little as possible?
One way of measuring these things is with your trusty linusometer. Manfred and I were sent back to the drawing board last week for a function which used 400 bytes...
Quoted 1 line
(and btw you have not even seen reiser4 stack usage ;) )
uh-oh. We need to be very sparing indeed.
I had a patch once which would print out "maximum stack space ever used by this process" on exit, but Alan fumbled it. I shall resurrect it.
Hello!
Quoted 7 lines · Andrew Morton
Quoted 4 lines
Well, 450 bytes is way below 4k (~7 times less if we'd take task struct into account) ;)
I can replace that on-stack array with kmalloc, but that probably would be a lot of overhead for no benefit.It would be a little overhead. kmalloc is damn quick, and remember that this data has to be copied from userspace and has go to disk
sometime. These things will make the kmalloc overhead very small.
Hm. Ok, this can be done.
Quoted 2 lines
Quoted 1 line
What do you think is safe stack usage limit for a function?
As little as possible?
reiserfs v3 was traditionally hungry on stack space I think.
Quoted 3 lines
One way of measuring these things is with your trusty linusometer. Manfred and I were sent back to the drawing board last week for a function which used 400 bytes...
;)
Quoted 5 lines
Quoted 1 line
(and btw you have not even seen reiser4 stack usage ;) )
uh-oh. We need to be very sparing indeed.
I had a patch once which would print out "maximum stack space ever used by this process" on exit, but Alan fumbled it. I shall resurrect it.
We have that for reiser4, that how we learn about all the stack overflows we have/had ;)
Bye,
Oleg
Quoted 1 line · Oleg Drokin
reiserfs v3 was traditionally hungry on stack space I think.
Well, if you want to drop stack usage, kill some inlines from stree.c. I really doubt we gain anything from inlining these:
fs/reiserfs/stree.c renamed from fs/reiserfs/stree.c.1 +3 -3
@@ -340,7 +340,7 @@/* Get delimiting key of the buffer at the path and its right neighbor. */ -inline const struct key *get_rkey ( +const struct key *get_rkey (const struct path * p_s_chk_path,const struct super_block * p_s_sb ) {
@@ -925,7 +925,7 @@// prepare for delete or cut of direct item -static inline int prepare_for_direct_item (struct path * path, +static int prepare_for_direct_item (struct path * path, struct item_head * le_ih, struct inode * inode, loff_t new_file_length,
@@ -970,7 +970,7 @@ } -static inline int prepare_for_direntry_item (struct path * path, +static int prepare_for_direntry_item (struct path * path, struct item_head * le_ih, struct inode * inode, loff_t new_file_length,
Hello!
Quoted 3 lines · Chris Mason
Quoted 1 line
reiserfs v3 was traditionally hungry on stack space I think.
Well, if you want to drop stack usage, kill some inlines from
stree.c. I really doubt we gain anything from inlining these:
I must be missing something, but I always thought that by inlining stuff we actually _decrease_ stack usage, because no need to create one more stackframe for function call, no need to put arguments on stack and this kind of stuff.
Bye,
Oleg
Quoted 8 lines · Andrew Morton
Quoted 5 lines
(and btw you have not even seen reiser4 stack usage ;) )
uh-oh. We need to be very sparing indeed.
It is not appropriate to reduce functionality and twist the code so as to reduce the stack, in my opinion. I suppose we'll get to that argument later though....