Re: [PATCH] erofs: make iov_iter describe target buffer when read from fscache

[Jingbo Xu]

Hi, Xiang,

On 1/22/24 3:42 PM, Gao Xiang wrote:
> Hi Jingbo,
> 
> On 2024/1/22 15:12, Jingbo Xu wrote:
>> So far the fscache mode supports uncompressed data only, and the data
>> read from fscache is put directly into the target page cache.  As the
>> support for compressed data in fscache mode is going to be introduced,
>> refactor the interface of reading fscache so that the following
>> compressed part could make the raw data read from fscache be directed to
>> the target buffer it wants, decompress the raw data, and finally fill
>> the page cache with the decompressed data.
>>
>> As the first step, a new structure, i.e. erofs_fscache_io (cio), is
> 
> I'd suggest just using io instead of cio here.
> 
> .. i.e. erofs_fscache_io (io) ...


Thanks for the review, I will update the patch regarding all these comments.


> 
>> introduced to describe a generic read request from the fscache, while
>> the caller can specify the target buffer it wants in the iov_iter
>> structure (cio->iter).  Besides, the caller can also specify its
> 
> .. structure (io->iter) ...
> 
>> completion callback and private data through cio, which will be called
>> to make further handling, e.g. unlocking the page cache for uncompressed
>> data or decompressing the read raw data, when the read request from the
>> fscache completes.  Now erofs_fscache_read_io_async() serves as a
>> generic interface for reading raw data from fscache for both compressed
>> and uncompressed data.
>>
>> The erofs_fscache_request structure is kept to describe a request to
>> fill the page cache in the specified range.
>>
>> Signed-off-by: Jingbo Xu <jefflexu@xxxxxxxxxxxxxxxxx>
>> ---
>>   fs/erofs/fscache.c | 219 ++++++++++++++++++++++++---------------------
>>   1 file changed, 118 insertions(+), 101 deletions(-)
>>
>> diff --git a/fs/erofs/fscache.c b/fs/erofs/fscache.c
>> index bc12030393b2..10709f20bef5 100644
>> --- a/fs/erofs/fscache.c
>> +++ b/fs/erofs/fscache.c
>> @@ -13,8 +13,6 @@ static LIST_HEAD(erofs_domain_cookies_list);
>>   static struct vfsmount *erofs_pseudo_mnt;
>>     struct erofs_fscache_request {
>> -    struct erofs_fscache_request *primary;
>> -    struct netfs_cache_resources cache_resources;
>>       struct address_space    *mapping;    /* The mapping being
>> accessed */
>>       loff_t            start;        /* Start position */
>>       size_t            len;        /* Length of the request */
>> @@ -23,42 +21,13 @@ struct erofs_fscache_request {
>>       refcount_t        ref;
>>   };
>>   -static struct erofs_fscache_request *erofs_fscache_req_alloc(struct
>> address_space *mapping,
>> -                         loff_t start, size_t len)
>> -{
>> -    struct erofs_fscache_request *req;
>> -
>> -    req = kzalloc(sizeof(struct erofs_fscache_request), GFP_KERNEL);
>> -    if (!req)
>> -        return ERR_PTR(-ENOMEM);
>> -
>> -    req->mapping = mapping;
>> -    req->start   = start;
>> -    req->len     = len;
>> -    refcount_set(&req->ref, 1);
>> -
>> -    return req;
>> -}
>> -
>> -static struct erofs_fscache_request *erofs_fscache_req_chain(struct
>> erofs_fscache_request *primary,
>> -                         size_t len)
>> -{
>> -    struct erofs_fscache_request *req;
>> -
>> -    /* use primary request for the first submission */
>> -    if (!primary->submitted) {
>> -        refcount_inc(&primary->ref);
>> -        return primary;
>> -    }
>> -
>> -    req = erofs_fscache_req_alloc(primary->mapping,
>> -            primary->start + primary->submitted, len);
>> -    if (!IS_ERR(req)) {
>> -        req->primary = primary;
>> -        refcount_inc(&primary->ref);
>> -    }
>> -    return req;
>> -}
>> +struct erofs_fscache_io {
>> +    struct netfs_cache_resources cache_resources;
> 
>     struct netfs_cache_resources cres;
> 
>> +    struct iov_iter        iter;
>> +    netfs_io_terminated_t    end_io;
>> +    void            *private;
>> +    refcount_t        ref;
>> +};
>>     static void erofs_fscache_req_complete(struct
>> erofs_fscache_request *req)
>>   {
>> @@ -83,82 +52,116 @@ static void erofs_fscache_req_complete(struct
>> erofs_fscache_request *req)
>>   static void erofs_fscache_req_put(struct erofs_fscache_request *req)
>>   {
>>       if (refcount_dec_and_test(&req->ref)) {
>> -        if (req->cache_resources.ops)
>> -           
>> req->cache_resources.ops->end_operation(&req->cache_resources);
>> -        if (!req->primary)
>> -            erofs_fscache_req_complete(req);
>> -        else
>> -            erofs_fscache_req_put(req->primary);
>> +        erofs_fscache_req_complete(req);
>>           kfree(req);
>>       }
>>   }
>>   -static void erofs_fscache_subreq_complete(void *priv,
>> +static struct erofs_fscache_request *erofs_fscache_req_alloc(struct
>> address_space *mapping,
>> +                        loff_t start, size_t len)
>> +{
>> +    struct erofs_fscache_request *req;
>> +
>> +    req = kzalloc(sizeof(*req), GFP_KERNEL);
>> +    if (req) {
>> +        req->mapping = mapping;
>> +        req->start = start;
>> +        req->len = len;
>> +        refcount_set(&req->ref, 1);
>> +    }
>> +    return req;
> 
> The following part may be better? to save an indentation:
> 
>     req = kzalloc(sizeof(*req), GFP_KERNEL);
>     if (!req)
>         return NULL;
>     req->mapping = mapping;
>     req->start = start;
>     req->len = len;
>     refcount_set(&req->ref, 1);
>     return req;
> 
>> +}
>> +
>> +static bool erofs_fscache_io_put(struct erofs_fscache_io *cio)
>> +{
>> +    if (refcount_dec_and_test(&cio->ref)) {
>> +        if (cio->cache_resources.ops)
>> +           
>> cio->cache_resources.ops->end_operation(&cio->cache_resources);
>> +        kfree(cio);
>> +        return true;
>> +    }
>> +    return false;
> 
> 
>     if (!refcount_dec_and_test(&io->ref))
>         return false;
>     if (io->cres.ops)
>         io->cres.ops->end_operation(&io->cres);
>     kfree(io);
>     return true;
> 
>> +}
>> +
>> +static void erofs_fscache_req_io_put(struct erofs_fscache_io *cio)
> 
> cio -> io
> 
>> +{
>> +    struct erofs_fscache_request *req = cio->private;
>> +
>> +    if (erofs_fscache_io_put(cio))
>> +        erofs_fscache_req_put(req);
>> +}
>> +
>> +static void erofs_fscache_req_end_io(void *priv,
>>           ssize_t transferred_or_error, bool was_async)
>>   {
>> -    struct erofs_fscache_request *req = priv;
>> +    struct erofs_fscache_io *cio = priv;
>> +    struct erofs_fscache_request *req = cio->private;
>> +
>> +    if (IS_ERR(transferred_or_error))
>> +        req->error = transferred_or_error;
>> +    erofs_fscache_req_io_put(cio);
>> +}
>> +
>> +static struct erofs_fscache_io *erofs_fscache_req_io_alloc(struct
>> erofs_fscache_request *req)
>> +{
>> +    struct erofs_fscache_io *cio;
>>   -    if (IS_ERR_VALUE(transferred_or_error)) {
>> -        if (req->primary)
>> -            req->primary->error = transferred_or_error;
>> -        else
>> -            req->error = transferred_or_error;
>> +    cio = kzalloc(sizeof(*cio), GFP_KERNEL);
>> +    if (cio) {
>> +        cio->end_io = erofs_fscache_req_end_io;
>> +        cio->private = req;
>> +        refcount_inc(&req->ref);
>> +        refcount_set(&cio->ref, 1);
>>       }
>> -    erofs_fscache_req_put(req);
>> +    return cio;
> 
> 
>     io = kzalloc(sizeof(*io), GFP_KERNEL);
>     if (!io)
>         return NULL;
>     io->end_io = erofs_fscache_req_end_io;
>     io->private = req;
>     refcount_inc(&req->ref);
>     refcount_set(&io->ref, 1);
>     return io;
> 
> Thanks,
> Gao Xiang

-- 
Thanks,
Jingbo