lkml.org 
[lkml]   [2006]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2006-11-01 17:51    [W:0.033 / U:209.980 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site