Re: [PATCH 08/13] RFC: pnfs: filelayout: introduce minimal file layout driver

Fred Isaman
2010-09-13T13:01:04Z

On Mon, Sep 13, 2010 at 3:32 AM, Benny Halevy <bhalevy@panasas.com> wrote:
> On 2010-09-11 01:37, Trond Myklebust wrote:
>> On Fri, 2010-09-10 at 14:11 -0700, Fred Isaman wrote:
>>> On Fri, Sep 10, 2010 at 12:31 PM, Trond Myklebust
>>> <Trond.Myklebust@netapp.com> wrote:
>>>> On Thu, 2010-09-02 at 14:00 -0400, Fred Isaman wrote:
>>> OK
>>>
>>>>> +     .initialize_mountpoint   = filelayout_initialize_mountpoint,
>>>>> +     .uninitialize_mountpoint = filelayout_uninitialize_mountpoint,
>>>>> +};
>>>>> +
>>>>> +
>>>>> +struct pnfs_layoutdriver_type filelayout_type = {
>>>>
>>>> Ditto.
>>>
>>> This includes a list_head field which is set by the generic layer.
>>>
>>>>
>>>>> +     .id = LAYOUT_NFSV4_1_FILES,
>>>>> +     .name = "LAYOUT_NFSV4_1_FILES",
>>>>> +     .ld_io_ops = &filelayout_io_operations,
>>>>
>>>> Why do we need a separate 'struct layoutdriver_io_operations'? Any
>>>> reason those can't just be embedded in struct pnfs_layoutdriver_type?
>>>
>>> I believe this decision was primarily aesthetics.  However, keeping
>>> the static io_ops seperate from the variable list_head seems like a
>>> good idea.
>>
>> I dunno. They are in a 1-1 correspondence, so I'm not sure I see the
>> need for a separation.
>>
>
> Later in the game we introduce the layout driver policy ops.
> That said, they could be added to the same vector as the io ops.

Yes, I think they should be merged when we get to that stage.

>
>>> Perhaps having a driver structure that includes the io_ops and static
>>> portions of pnfs_layoutdriver_type, with the generic layer allocating
>>> a wrapper structure that is basically:
>>> struct {
>>>     struct list_head list;
>>>     struct pnfs_layoutdriver_type *driver_info;
>>
>>       Should be const...
>>
>>       struct module *owner = THIS_MODULE;
>>
>>> }
>>
>> ...although the struct module could probably indeed be part of
>> pnfs_layoutdriver_type too.
>
> Agreed.
> I think we should just have
>
> struct pnfs_layoutdriver_type {
>        struct list_head pnfs_tblid;
>        const u32 id;
>        const char *name;
>        const struct module *owner;
>        const struct layoutdriver_operations *ld_ops;
>  };
>
> Benny
>

I'll point out that what I took from the above conversation, was that
the fields id, name, and possibly owner should be placed inside struct
layoutdriver_operations (which should probably be renamed slightly).

>>
>> Cheers
>>   Trond
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>