Re: [fuse-devel] [PATCH 07/11] fuse: add per-page descriptor <offset, length> to fuse_req

[Brian F. <bf...@re...>]

On 09/26/2012 05:52 AM, Maxim V. Patlasov wrote:
> Brian,
> 
>>> +/** FUSE page descriptor */
>>> +struct page_desc {
>>> +    unsigned int length;
>>> +    unsigned int offset;
>>> +};
>>> +
>> struct fuse_page_desc ?
> 
> Good point, thanks! I'll wait for a while for more comments and resend
> the patch then.
> 
>>>   /** The request state */
>>>   enum fuse_req_state {
>>>       FUSE_REQ_INIT = 0,
>>> @@ -296,18 +302,21 @@ struct fuse_req {
>>>       /** page vector */
>>>       struct page **pages;
>>>   +    /** page-descriptor vector */
>>> +    struct page_desc *page_descs;
>>> +
>>>       /** size of the 'pages' array */
>>>       unsigned max_pages;
>>>         /** inline page vector */
>>>       struct page *inline_pages[FUSE_REQ_INLINE_PAGES];
>>>   +    /** inline page-descriptor vector */
>>> +    struct page_desc inline_page_descs[FUSE_REQ_INLINE_PAGES];
>>> +
>>>       /** number of pages in vector */
>>>       unsigned num_pages;
>>>   
>> More aesthetic than anything I suppose, but any reason not to fold the
>> page pointers into the descriptor? It seems like it would be slightly
>> more simple.
> 
> Yes, this was my initial intention. But then I stumbled over
> get_user_pages_fast() and friends. They all want plain array of page
> pointers. Having considered several approaches, this one (adding array
> of page_desc structures) looked lesser evil than others. There was some
> discussion in a thread titled '[PATCH 0/6] fuse: allocate req->pages[]
> dynamically' a week ago.
> 

Ah, good point. I think I missed the original discussion around v1 of
this set. I'll go back and skim through it. Thanks.

Brian

> Thanks,
> Maxim