On Wed, 2011-03-23 at 22:39 +0200, Benny Halevy wrote:
> On 2011-03-23 22:30, Fred Isaman wrote:
> > 2011/3/23 Benny Halevy <bhalevy@xxxxxxxxxxx>:
> >>> +}
> >>> +
> >>> +static inline struct list_head *
> >>> +pnfs_choose_commit_list(struct nfs_page *req, struct list_head *mds)
> >>> +{
> >>> + struct list_head *rv;
> >>> +
> >>> + if (test_and_clear_bit(PG_PNFS_COMMIT,&req->wb_flags)) {
> >>
> >> nit: space after comma
> >>
> >>> + struct inode *inode =
> >>> req->wb_commit_lseg->pls_layout->plh_inode;
> >>> +
> >>> + set_bit(NFS_INO_PNFS_COMMIT,&NFS_I(inode)->flags);
> >>
> >> nit: ditto
> >>
> >>> + rv =
> >>> NFS_SERVER(inode)->pnfs_curr_ld->choose_commit_list(req);
> >>> + /* matched by ref taken when PG_PNFS_COMMIT is set */
> >>> + put_lseg(req->wb_commit_lseg);
> >>
> >> Since wb_commit_lseg and wb_list are in a union, how about
> >> INIT_LIST_HEAD(&req->wb_list);
> >
> > I actually had that there. Trond pointed out it was unnecessary and
> > asked that it be removed.
> >
>
> Seems fragile to me. I hope Trond's around to fix it when it breaks ;-)
Why would it break? The point is that you are initialising the list
header and then immediately clobbering that initialisation by adding it
to a list.
It's like initialising 'foo' to the value 0 and then immediately
assigning the value 1. That's not defensive programming, just a waste of
CPU cycles (or compiler cycles if the compiler is smart).
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
