Re: [PATCH v12 5/5] cachefiles: Preset the state xattr when creating a new file

[Paulo Alcantara]

David Howells <dhowells@xxxxxxxxxx> writes:

> With a really small cache, cachefiles is likely to see a lot of writes
> hitting ENOSPC - and this can include setxattr that sets the state xattr on
> a cachefile - but we don't really want to successfully fill a cache file
> only to have to scrap it because we can't set the xattr.
>
> Instead, preset the xattr when we create the tmpfile we're going to use,
> and scrap the file at that point if we get ENOSPC.  Only if setxattr
> succeeds do we allow data to be written to the file.
>
> Note that there is a potential performance loss in that writes to the cache
> have to be delayed until this is completed - but we do the tmpfile/setxattr
> in parallel, starting when the file is opened and only have to wait once
> writeback occurs.
>
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> cc: Marc Dionne <marc.dionne@xxxxxxxxxxxx>
> cc: Paulo Alcantara <pc@xxxxxxxxxxxxx>
> cc: netfs@xxxxxxxxxxxxxxx
> cc: linux-fsdevel@xxxxxxxxxxxxxxx
> ---
>  fs/cachefiles/internal.h |  1 +
>  fs/cachefiles/namei.c    |  5 ++++
>  fs/cachefiles/xattr.c    | 59 +++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 64 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cachefiles/internal.h b/fs/cachefiles/internal.h
> index b2605111fd56..664be64ab538 100644
> --- a/fs/cachefiles/internal.h
> +++ b/fs/cachefiles/internal.h
> @@ -283,6 +283,7 @@ void cachefiles_withdraw_volume(struct cachefiles_volume *volume);
>  /*
>   * xattr.c
>   */
> +int cachefiles_preset_object_xattr(struct cachefiles_object *object, struct file *file);
>  extern int cachefiles_set_object_xattr(struct cachefiles_object *object);
>  extern int cachefiles_check_auxdata(struct cachefiles_object *object,
>  				    struct file *file);
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index ca093840e577..ef656a319ede 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -450,6 +450,11 @@ struct file *cachefiles_create_tmpfile(struct cachefiles_object *object)
>  		pr_notice("Cache does not support read_iter and write_iter\n");
>  		goto err_unuse;
>  	}
> +
> +	/* Preallocate space for the xattr. */
> +	ret = cachefiles_preset_object_xattr(object, file);
> +	if (ret < 0)
> +		goto err_unuse;
>  out:
>  	cachefiles_end_secure(cache, saved_cred);
>  	object->content_info = CACHEFILES_CONTENT_ALL;
> diff --git a/fs/cachefiles/xattr.c b/fs/cachefiles/xattr.c
> index 551a3b0069c2..8ebb713482e3 100644
> --- a/fs/cachefiles/xattr.c
> +++ b/fs/cachefiles/xattr.c
> @@ -34,6 +34,57 @@ struct cachefiles_vol_xattr {
>  	__u8	data[];		/* netfs volume coherency data */
>  } __packed;
>  
> +/*
> + * Preset the state xattr on a cache file to allocate space for it.
> + */
> +int cachefiles_preset_object_xattr(struct cachefiles_object *object, struct file *file)
> +{
> +	struct cachefiles_xattr *buf;
> +	struct dentry *dentry = file->f_path.dentry;
> +	unsigned int len = object->cookie->aux_len;
> +	int ret;
> +
> +	buf = kzalloc(sizeof(struct cachefiles_xattr) + min(len, sizeof(__be64)), GFP_KERNEL);

You should use max() here.

Otherwise looks good,

Reviewed-by: Paulo Alcantara <pc@xxxxxxxxxxxxx>