On Fri, 2011-06-10 at 09:14 -0700, Boaz Harrosh wrote:
> On 06/09/2011 09:43 PM, Benny Halevy wrote:
> > On 2011-06-10 00:28, Boaz Harrosh wrote:
> >> On 06/09/2011 09:06 PM, Benny Halevy wrote:
> >>> On 2011-06-09 22:53, Boaz Harrosh wrote:
> >>>> On 06/09/2011 06:31 PM, Trond Myklebust wrote:
> >>>> <snip>
> >>>>> +void
> >>>>> +pnfs_generic_pg_init_read(struct nfs_pageio_descriptor *pgio, struct nfs_page *req)
> >>>>> +{
> >>>>> + BUG_ON(pgio->pg_lseg != NULL);
> >>>>> +
> >>>>> + pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode,
> >>>>> + req->wb_context,
> >>>>> + req_offset(req),
> >>>>> + req->wb_bytes,
> >>>>> + IOMODE_READ,
> >>>>> + GFP_KERNEL);
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(pnfs_generic_pg_init_read);
> >>>>> +
> >>>>> +void
> >>>>> +pnfs_generic_pg_init_write(struct nfs_pageio_descriptor *pgio, struct nfs_page *req)
> >>>>> +{
> >>>>> + BUG_ON(pgio->pg_lseg != NULL);
> >>>>> +
> >>>>> + pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode,
> >>>>> + req->wb_context,
> >>>>> + req_offset(req),
> >>>>> + req->wb_bytes,
> >>>>> + IOMODE_RW,
> >>>>> + GFP_NOFS);
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(pnfs_generic_pg_init_write);
> >>>>> +
> >>>>
> >>>> These two above are identical except the IOMODE_{READ,RW} variable.
> >>>
> >>> And the respective gfp flags...
> >>>
> >>
> >> So is that "we should" or should-not?
> >>
> >
> > That doesn't make much sense when you've defined separate vectors
> > for read and write.
>
> So what the same function is set at both vectors, and the few callers (1)
> passes a flag.
>
> If you ask me we could do with one vector and a flag throughout this
> stack. Actually don't make the flag as function parameter but as a member
> of nfs_pageio_descriptor. For example in osd we want to know if we are
> reading or writing in the raid5 case it produces different results.
>
> > And in any case, since pg_init is not pnfs specific the caller shouldn't
> > pass IOMODE_* values but rather a generic value that will require translation anyway.
> > In this case I'd just consider using a common function to be called
> > respectively from both methods:
> >
> > static void
> > pnfs_pg_update_layout(struct nfs_pageio_descriptor *pgio, struct nfs_page *req,
> > enum pnfs_iomode iomode, gfp_t gfp_flags)
> > {
> > BUG_ON(pgio->pg_lseg != NULL);
> >
> > pgio->pg_lseg = pnfs_update_layout(pgio->pg_inode,
> > req->wb_context,
> > req_offset(req),
> > req->wb_bytes,
> > iomode,
> > gfp_flags);
> > }
> >
>
> That makes it even more complicated for a do nothing function. We dont do
> a different function for each different parameter. We can just do a
> "bool write" and unify the dam thing
Right now, the nfs_pageio_descriptor doesn't know about reads vs writes.
It just knows about 'coalesce requests' and 'perform i/o'. I'd prefer to
keep that abstraction, as it makes things cleaner, particularly when you
get to patch 5 (NFSv4.1: Fall back to ordinary i/o through the mds if we
have no layout segment). Why add more 'if' statements when you don't
need to...
--
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
