Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised again

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/1/06, Jörn Engel <joern@xxxxxxxxxxxxxxxxxxxx> 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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux