From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Holden Karau" Subject: Re: [PATCH 1/1] fat: improve sync performance by grouping writes revised again Date: Wed, 1 Nov 2006 13:02:12 -0500 Message-ID: References: <4548C8AE.2090603@pigscanfly.ca> <20061101164715.GC16154@wohnheim.fh-wedel.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "Josef Sipek" , hirofumi@mail.parknet.co.jp, linux-kernel@vger.kernel.org, "Holden Karau" , "akpm@osdl.org" , linux-fsdevel@vger.kernel.org, "Nick Piggin" , "Matthew Wilcox" Return-path: Received: from nf-out-0910.google.com ([64.233.182.186]:36266 "EHLO nf-out-0910.google.com") by vger.kernel.org with ESMTP id S1752233AbWKASCP convert rfc822-to-8bit (ORCPT ); Wed, 1 Nov 2006 13:02:15 -0500 Received: by nf-out-0910.google.com with SMTP id c2so923738nfe for ; Wed, 01 Nov 2006 10:02:14 -0800 (PST) To: "=?ISO-8859-1?Q?J=F6rn_Engel?=" In-Reply-To: <20061101164715.GC16154@wohnheim.fh-wedel.de> Content-Disposition: inline Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On 11/1/06, J=F6rn Engel wrote: > On Wed, 1 November 2006 11:17:50 -0500, Holden Karau wrote: > > + c_bh =3D kmalloc(nr_bhs*(sbi->fats) , GFP_KERNEL); > > + if (NULL =3D=3D c_bh) { > > + printk(KERN_CRIT "not enough memory to store pointers= to FAT blocks, will not sync. Possible data loss\n"); > > + err =3D -ENOMEM; > > + goto error; > > + } > > o I personally hate Yoda code ("Null the pointer is not, young Jedi")= =2E > 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 =3D kmalloc(... > err =3D -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 =3D 0 ; n < nr_bhs ; n++ ) { > > + c_bh[(copy-1)*nr_bhs+n] =3D sb_getblk(sb, bac= kup_fat + bhs[n]->b_blocknr); > > + /* If there is not enough memory, fall back t= o the old system */ > > + if (!c_bh[(copy-1)*nr_bhs+n]) { > > + printk("fat: not enough memory for al= l 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] =3D sb_getblk= (sb, backup_fat + bhs[n]->b_blocknr); > > + if (!c_bh[(copy-1)*nr_bhs+n]) { > > + printk(KERN_CRIT "fat:not eno= ugh memory for block after existing blocks released. Possible data loss= =2E\n"); Based on the same reasoning you provided, I should probably drop this o= ne too. > > + err =3D -ENOMEM; > > + goto error; > > + } > > As above. I'll drop the printk, but the same holds true about err > > > error: > > + if (NULL !=3D 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 wa= it) > > { > > int i, err =3D 0; > > > > ll_rw_block(SWRITE, nr_bhs, bhs); > > - for (i =3D 0; i < nr_bhs; i++) { > > - wait_on_buffer(bhs[i]); > > - if (buffer_eopnotsupp(bhs[i])) { > > - clear_buffer_eopnotsupp(bhs[i]); > > - err =3D -EOPNOTSUPP; > > - } else if (!err && !buffer_uptodate(bhs[i])) > > - err =3D -EIO; > > + if (wait) { > > + for (i =3D 0; i < nr_bhs; i++) { > > + wait_on_buffer(bhs[i]); > > + if (buffer_eopnotsupp(bhs[i])) { > > + clear_buffer_eopnotsupp(bhs[i]); > > + err =3D -EOPNOTSUPP; > > + } else if (!err && !buffer_uptodate(bhs[i])) > > + err =3D -EIO; > > + } > > } > > + > > return err; > > } > > You could keep the old indentation if your condition was changed to > > if (!wait) > return 0; Sounds good. > > J=F6rn > > -- > You can take my soul, but not my lack of enthusiasm. > -- Wally > --=20 Cell: 613-276-1645 - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html