Messages in this thread | | | Date | Wed, 1 Nov 2006 13:02:12 -0500 | From | "Holden Karau" <> | Subject | Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised again |
| |
On 11/1/06, Jörn Engel <joern@wohnheim.fh-wedel.de> wrote: > 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. Ok, I'll drop the printk > 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; That wouldn't work so well since we always return err, and possibly slightly better code for i386 doesn't seem all that worth it. > > > + 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. I suppose I might as well drop it. > > > + 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"); Based on the same reasoning you provided, I should probably drop this one too. > > + err = -ENOMEM; > > + goto error; > > + } > > As above. I'll drop the printk, but the same holds true about err > > > error: > > + if (NULL != c_bh) { > > + kfree(c_bh); > > + } > > kfree(NULL) works just fine. You can remove the condition. Thanks, I should have checked that :-) > > > +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; Sounds good. > > Jörn > > -- > You can take my soul, but not my lack of enthusiasm. > -- Wally >
-- Cell: 613-276-1645 - 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/
| |