Messages in this thread | | | Date | Wed, 1 Nov 2006 17:47:15 +0100 | From | Jörn Engel <> | Subject | Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised again |
| |
On Wed, 1 November 2006 11:17:50 -0500, Holden Karau wrote: > + c_bh = kmalloc(nr_bhs*(sbi->fats) , GFP_KERNEL); > + if (NULL == c_bh) { > + printk(KERN_CRIT "not enough memory to store pointers to FAT blocks, will not sync. Possible data loss\n"); > + err = -ENOMEM; > + goto error; > + }
o I personally hate Yoda code ("Null the pointer is not, young Jedi"). o Old code simply returned -ENOMEM without printk. Assuming this was sufficient before, the printk can be dropped. o Some people prefer assigning err outside the condition. It is supposed to give slightly better code on i386, iirc.
Result would be something like: c_bh = kmalloc(... err = -ENOMEM; if (!c_bh) goto error;
> + for (n = 0 ; n < nr_bhs ; n++ ) { > + c_bh[(copy-1)*nr_bhs+n] = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr); > + /* If there is not enough memory, fall back to the old system */ > + if (!c_bh[(copy-1)*nr_bhs+n]) { > + printk("fat: not enough memory for all blocks , syncing at %d\n" ,(copy-1)*nr_bhs+n);
Whether this printk makes sense, I cannot tell.
> + fat_sync_bhs_optw( c_bh+i , (copy-1)*nr_bhs+n-i-1 , wait ); > + /* Free the now sync'd blocks */ > + for (; i < (copy-1)*nr_bhs+n ; i++) > + brelse(c_bh[i]); > + /* We try the same block again */ > + c_bh[(copy-1)*nr_bhs+n] = sb_getblk(sb, backup_fat + bhs[n]->b_blocknr); > + if (!c_bh[(copy-1)*nr_bhs+n]) { > + printk(KERN_CRIT "fat:not enough memory for block after existing blocks released. Possible data loss.\n"); > + err = -ENOMEM; > + goto error; > + }
As above.
> error: > + if (NULL != c_bh) { > + kfree(c_bh); > + }
kfree(NULL) works just fine. You can remove the condition.
> +int fat_sync_bhs_optw(struct buffer_head **bhs, int nr_bhs ,int wait) > { > int i, err = 0; > > ll_rw_block(SWRITE, nr_bhs, bhs); > - for (i = 0; i < nr_bhs; i++) { > - wait_on_buffer(bhs[i]); > - if (buffer_eopnotsupp(bhs[i])) { > - clear_buffer_eopnotsupp(bhs[i]); > - err = -EOPNOTSUPP; > - } else if (!err && !buffer_uptodate(bhs[i])) > - err = -EIO; > + if (wait) { > + for (i = 0; i < nr_bhs; i++) { > + wait_on_buffer(bhs[i]); > + if (buffer_eopnotsupp(bhs[i])) { > + clear_buffer_eopnotsupp(bhs[i]); > + err = -EOPNOTSUPP; > + } else if (!err && !buffer_uptodate(bhs[i])) > + err = -EIO; > + } > } > + > return err; > }
You could keep the old indentation if your condition was changed to
if (!wait) return 0;
Jörn
-- You can take my soul, but not my lack of enthusiasm. -- Wally - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |