Add prepare_ondemand_read() callback dedicated for the on-demand read
scenario, so that callers from this scenario can be decoupled from
netfs_io_subrequest.
To reuse the hole detecting logic as mush as possible, both the
implementation of prepare_read() and prepare_ondemand_read() inside
Cachefiles call a common routine.
In the near future, prepare_read() will get enhanced and more
information will be needed and then returned to callers. Thus
netfs_io_subrequest is a reasonable candidate for holding places for all
these information needed in the internal implementation.
Signed-off-by: Jingbo Xu <[email protected]>
---
fs/cachefiles/io.c | 42 +++++++++++++++++++++++++------
include/linux/netfs.h | 7 ++++++
include/trace/events/cachefiles.h | 4 +--
3 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
index 000a28f46e59..6427259fcba9 100644
--- a/fs/cachefiles/io.c
+++ b/fs/cachefiles/io.c
@@ -385,16 +385,11 @@ static int cachefiles_write(struct netfs_cache_resources *cres,
term_func, term_func_priv);
}
-/*
- * Prepare a read operation, shortening it to a cached/uncached
- * boundary as appropriate.
- */
-static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq,
- loff_t i_size)
+static enum netfs_io_source cachefiles_do_prepare_read(struct netfs_io_subrequest *subreq,
+ struct netfs_cache_resources *cres,
+ loff_t i_size)
{
enum cachefiles_prepare_read_trace why;
- struct netfs_io_request *rreq = subreq->rreq;
- struct netfs_cache_resources *cres = &rreq->cache_resources;
struct cachefiles_object *object;
struct cachefiles_cache *cache;
struct fscache_cookie *cookie = fscache_cres_cookie(cres);
@@ -501,6 +496,36 @@ static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *
return ret;
}
+/*
+ * Prepare a read operation, shortening it to a cached/uncached
+ * boundary as appropriate.
+ */
+static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq,
+ loff_t i_size)
+{
+ return cachefiles_do_prepare_read(subreq,
+ &subreq->rreq->cache_resources, i_size);
+}
+
+/*
+ * Prepare an on-demand read operation, shortening it to a cached/uncached
+ * boundary as appropriate.
+ */
+static enum netfs_io_source cachefiles_prepare_ondemand_read(struct netfs_cache_resources *cres,
+ loff_t start, size_t *_len, loff_t i_size)
+{
+ enum netfs_io_source source;
+ struct netfs_io_subrequest subreq = {
+ .start = start,
+ .len = *_len,
+ .flags = 1 << NETFS_SREQ_ONDEMAND,
+ };
+
+ source = cachefiles_do_prepare_read(&subreq, cres, i_size);
+ *_len = subreq.len;
+ return source;
+}
+
/*
* Prepare for a write to occur.
*/
@@ -621,6 +646,7 @@ static const struct netfs_cache_ops cachefiles_netfs_cache_ops = {
.write = cachefiles_write,
.prepare_read = cachefiles_prepare_read,
.prepare_write = cachefiles_prepare_write,
+ .prepare_ondemand_read = cachefiles_prepare_ondemand_read,
.query_occupancy = cachefiles_query_occupancy,
};
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index f2402ddeafbf..d82071c37133 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -267,6 +267,13 @@ struct netfs_cache_ops {
loff_t *_start, size_t *_len, loff_t i_size,
bool no_space_allocated_yet);
+ /* Prepare an on-demand read operation, shortening it to a cached/uncached
+ * boundary as appropriate.
+ */
+ enum netfs_io_source (*prepare_ondemand_read)(struct netfs_cache_resources *cres,
+ loff_t start, size_t *_len,
+ loff_t i_size);
+
/* Query the occupancy of the cache in a region, returning where the
* next chunk of data starts and how long it is.
*/
diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h
index d8d4d73fe7b6..655d5900b8ef 100644
--- a/include/trace/events/cachefiles.h
+++ b/include/trace/events/cachefiles.h
@@ -448,14 +448,14 @@ TRACE_EVENT(cachefiles_prep_read,
),
TP_fast_assign(
- __entry->rreq = sreq->rreq->debug_id;
+ __entry->rreq = sreq->rreq ? sreq->rreq->debug_id : 0;
__entry->index = sreq->debug_index;
__entry->flags = sreq->flags;
__entry->source = source;
__entry->why = why;
__entry->len = sreq->len;
__entry->start = sreq->start;
- __entry->netfs_inode = sreq->rreq->inode->i_ino;
+ __entry->netfs_inode = sreq->rreq ? sreq->rreq->inode->i_ino : 0;
__entry->cache_inode = cache_inode;
),
--
2.19.1.6.gb485710b
On Fri, 2022-11-04 at 15:26 +0800, Jingbo Xu wrote:
> Add prepare_ondemand_read() callback dedicated for the on-demand read
> scenario, so that callers from this scenario can be decoupled from
> netfs_io_subrequest.
>
> To reuse the hole detecting logic as mush as possible, both the
> implementation of prepare_read() and prepare_ondemand_read() inside
> Cachefiles call a common routine.
>
> In the near future, prepare_read() will get enhanced and more
> information will be needed and then returned to callers. Thus
> netfs_io_subrequest is a reasonable candidate for holding places for all
> these information needed in the internal implementation.
>
> Signed-off-by: Jingbo Xu <[email protected]>
> ---
> fs/cachefiles/io.c | 42 +++++++++++++++++++++++++------
> include/linux/netfs.h | 7 ++++++
> include/trace/events/cachefiles.h | 4 +--
> 3 files changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
> index 000a28f46e59..6427259fcba9 100644
> --- a/fs/cachefiles/io.c
> +++ b/fs/cachefiles/io.c
> @@ -385,16 +385,11 @@ static int cachefiles_write(struct netfs_cache_resources *cres,
> term_func, term_func_priv);
> }
>
> -/*
> - * Prepare a read operation, shortening it to a cached/uncached
> - * boundary as appropriate.
> - */
> -static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq,
> - loff_t i_size)
> +static enum netfs_io_source cachefiles_do_prepare_read(struct netfs_io_subrequest *subreq,
> + struct netfs_cache_resources *cres,
> + loff_t i_size)
> {
> enum cachefiles_prepare_read_trace why;
> - struct netfs_io_request *rreq = subreq->rreq;
> - struct netfs_cache_resources *cres = &rreq->cache_resources;
> struct cachefiles_object *object;
> struct cachefiles_cache *cache;
> struct fscache_cookie *cookie = fscache_cres_cookie(cres);
> @@ -501,6 +496,36 @@ static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *
> return ret;
> }
>
> +/*
> + * Prepare a read operation, shortening it to a cached/uncached
> + * boundary as appropriate.
> + */
> +static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq,
> + loff_t i_size)
> +{
> + return cachefiles_do_prepare_read(subreq,
> + &subreq->rreq->cache_resources, i_size);
> +}
> +
> +/*
> + * Prepare an on-demand read operation, shortening it to a cached/uncached
> + * boundary as appropriate.
> + */
> +static enum netfs_io_source cachefiles_prepare_ondemand_read(struct netfs_cache_resources *cres,
> + loff_t start, size_t *_len, loff_t i_size)
> +{
> + enum netfs_io_source source;
> + struct netfs_io_subrequest subreq = {
> + .start = start,
> + .len = *_len,
> + .flags = 1 << NETFS_SREQ_ONDEMAND,
> + };
> +
Faking up a struct like this is sort of fragile. What if we change
cachefiles_do_prepare_read to consult other fields in this structure
later?
It might be best to have cachefiles_do_prepare_read take individual
start, len, and flags values instead of doing this.
> + source = cachefiles_do_prepare_read(&subreq, cres, i_size);
> + *_len = subreq.len;
> + return source;
> +}
> +
> /*
> * Prepare for a write to occur.
> */
> @@ -621,6 +646,7 @@ static const struct netfs_cache_ops cachefiles_netfs_cache_ops = {
> .write = cachefiles_write,
> .prepare_read = cachefiles_prepare_read,
> .prepare_write = cachefiles_prepare_write,
> + .prepare_ondemand_read = cachefiles_prepare_ondemand_read,
> .query_occupancy = cachefiles_query_occupancy,
> };
>
> diff --git a/include/linux/netfs.h b/include/linux/netfs.h
> index f2402ddeafbf..d82071c37133 100644
> --- a/include/linux/netfs.h
> +++ b/include/linux/netfs.h
> @@ -267,6 +267,13 @@ struct netfs_cache_ops {
> loff_t *_start, size_t *_len, loff_t i_size,
> bool no_space_allocated_yet);
>
> + /* Prepare an on-demand read operation, shortening it to a cached/uncached
> + * boundary as appropriate.
> + */
> + enum netfs_io_source (*prepare_ondemand_read)(struct netfs_cache_resources *cres,
> + loff_t start, size_t *_len,
> + loff_t i_size);
> +
> /* Query the occupancy of the cache in a region, returning where the
> * next chunk of data starts and how long it is.
> */
> diff --git a/include/trace/events/cachefiles.h b/include/trace/events/cachefiles.h
> index d8d4d73fe7b6..655d5900b8ef 100644
> --- a/include/trace/events/cachefiles.h
> +++ b/include/trace/events/cachefiles.h
> @@ -448,14 +448,14 @@ TRACE_EVENT(cachefiles_prep_read,
> ),
>
> TP_fast_assign(
> - __entry->rreq = sreq->rreq->debug_id;
> + __entry->rreq = sreq->rreq ? sreq->rreq->debug_id : 0;
> __entry->index = sreq->debug_index;
> __entry->flags = sreq->flags;
> __entry->source = source;
> __entry->why = why;
> __entry->len = sreq->len;
> __entry->start = sreq->start;
> - __entry->netfs_inode = sreq->rreq->inode->i_ino;
> + __entry->netfs_inode = sreq->rreq ? sreq->rreq->inode->i_ino : 0;
> __entry->cache_inode = cache_inode;
> ),
>
--
Jeff Layton <[email protected]>
On 11/4/22 7:18 PM, Jeff Layton wrote:
> On Fri, 2022-11-04 at 15:26 +0800, Jingbo Xu wrote:
>> Add prepare_ondemand_read() callback dedicated for the on-demand read
>> scenario, so that callers from this scenario can be decoupled from
>> netfs_io_subrequest.
>>
>> To reuse the hole detecting logic as mush as possible, both the
>> implementation of prepare_read() and prepare_ondemand_read() inside
>> Cachefiles call a common routine.
>>
>> In the near future, prepare_read() will get enhanced and more
>> information will be needed and then returned to callers. Thus
>> netfs_io_subrequest is a reasonable candidate for holding places for all
>> these information needed in the internal implementation.
>>
>> Signed-off-by: Jingbo Xu <[email protected]>
>> ---
>> fs/cachefiles/io.c | 42 +++++++++++++++++++++++++------
>> include/linux/netfs.h | 7 ++++++
>> include/trace/events/cachefiles.h | 4 +--
>> 3 files changed, 43 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
>> index 000a28f46e59..6427259fcba9 100644
>> --- a/fs/cachefiles/io.c
>> +++ b/fs/cachefiles/io.c
>> @@ -385,16 +385,11 @@ static int cachefiles_write(struct netfs_cache_resources *cres,
>> term_func, term_func_priv);
>> }
>>
>> -/*
>> - * Prepare a read operation, shortening it to a cached/uncached
>> - * boundary as appropriate.
>> - */
>> -static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq,
>> - loff_t i_size)
>> +static enum netfs_io_source cachefiles_do_prepare_read(struct netfs_io_subrequest *subreq,
>> + struct netfs_cache_resources *cres,
>> + loff_t i_size)
>> {
>> enum cachefiles_prepare_read_trace why;
>> - struct netfs_io_request *rreq = subreq->rreq;
>> - struct netfs_cache_resources *cres = &rreq->cache_resources;
>> struct cachefiles_object *object;
>> struct cachefiles_cache *cache;
>> struct fscache_cookie *cookie = fscache_cres_cookie(cres);
>> @@ -501,6 +496,36 @@ static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *
>> return ret;
>> }
>>
>> +/*
>> + * Prepare a read operation, shortening it to a cached/uncached
>> + * boundary as appropriate.
>> + */
>> +static enum netfs_io_source cachefiles_prepare_read(struct netfs_io_subrequest *subreq,
>> + loff_t i_size)
>> +{
>> + return cachefiles_do_prepare_read(subreq,
>> + &subreq->rreq->cache_resources, i_size);
>> +}
>> +
>> +/*
>> + * Prepare an on-demand read operation, shortening it to a cached/uncached
>> + * boundary as appropriate.
>> + */
>> +static enum netfs_io_source cachefiles_prepare_ondemand_read(struct netfs_cache_resources *cres,
>> + loff_t start, size_t *_len, loff_t i_size)
>> +{
>> + enum netfs_io_source source;
>> + struct netfs_io_subrequest subreq = {
>> + .start = start,
>> + .len = *_len,
>> + .flags = 1 << NETFS_SREQ_ONDEMAND,
>> + };
>> +
>
> Faking up a struct like this is sort of fragile. What if we change
> cachefiles_do_prepare_read to consult other fields in this structure
> later?
Indeed it's not robust somehow.
>
> It might be best to have cachefiles_do_prepare_read take individual
> start, len, and flags values instead of doing this.
>
I will give it a try if nobody objects this.
Thank you for your suggestions :)
--
Thanks,
Jingbo