Re: [RESEND4, PATCH 1/2] fuse: retrieve: cap requested size to negotiated max_write

[Kirill Smelkov]

On Wed, Apr 24, 2019 at 03:19:11PM +0200, Miklos Szeredi wrote:
> On Wed, Apr 24, 2019 at 2:31 PM Kirill Smelkov <kirr@xxxxxxxxxx> wrote:
>
> > Thanks. Does it mean that the patch is ok? Do I need to rework
> > something?
>
> Pushed to #for-next with all the rest.  Made some changes to the
> patches, so please verify.

Thanks a lot. I've verified all changes and it is indeed better to amend
something:

- FOPEN_STREAM:

    --- b/include/uapi/linux/fuse.h
    +++ b/include/uapi/linux/fuse.h
    @@ -232,7 +232,7 @@
      * FOPEN_KEEP_CACHE: don't invalidate the data cache on open
      * FOPEN_NONSEEKABLE: the file is not seekable
      * FOPEN_CACHE_DIR: allow caching this directory
    - * FOPEN_STREAM: the file is stream-like
    + * FOPEN_STREAM: the file is stream-like (no file position at all)
      */
     #define FOPEN_DIRECT_IO                (1 << 0)
     #define FOPEN_KEEP_CACHE       (1 << 1)

I agree, it is better this way (no amendment needed here).


- FUSE_PRECISE_INVAL_DATA:

    --- b/include/uapi/linux/fuse.h
    +++ b/include/uapi/linux/fuse.h
    @@ -266,7 +266,7 @@
      * FUSE_MAX_PAGES: init_out.max_pages contains the max number of req pages
      * FUSE_CACHE_SYMLINKS: cache READLINK responses
      * FUSE_NO_OPENDIR_SUPPORT: kernel supports zero-message opendir
    - * FUSE_PRECISE_INVAL_DATA: filesystem is fully responsible for data cache invalidation
    + * FUSE_PRECISE_INVAL_DATA: filesystem is fully responsible for invalidation
      */
     #define FUSE_ASYNC_READ                (1 << 0)
     #define FUSE_POSIX_LOCKS       (1 << 1)

the "data cache" in "for data cache invalidation" has particular meaning
and semantic: the filesystem promises to explicitly invalidate data of
the file, but it does not promise to explicitly invalidate attributes. I
understand it is a long line, and I myself tried to remove extra words,
but "data cache" here is semantically needed, so I left it. The
particular behaviour of FUSE_PRECISE_INVAL_DATA also covers only data
cache invalidations. For example file attributes, if not explicitly
invalidated by filesystem, are still invalidated by kernel by its
heuristic and due to negotiated attributes timeout, which is not
"precise".

If it is "precise invalidation for everything: data and attributes" we
should probably rename it to FUSE_PRECISE_INVAL and change the patch to
also cover attributes and not invalidate them automatically. However I
suggest to keep two things separate - data and attributes and not to
change the patch. By the way, description for "auto" invalidation mode is

	FUSE_AUTO_INVAL_DATA: automatically invalidate cached pages

which tells both from mode name and from its description that it is
about data.

uapi/linux/fuse.h is often used by userspace people as a document to
understand FUSE protocol (at least I used it this way). I thus suggest
to restore "data cache" since it makes semantic difference.

Your amendment for FOPEN_STREAM in uapi/linux/fuse.h (see above) also
suggests that it is better to be more explicit in that file.

    --- b/fs/fuse/inode.c
    +++ b/fs/fuse/inode.c
    @@ -913,13 +913,8 @@
                                    fc->dont_mask = 1;
                            if (arg->flags & FUSE_AUTO_INVAL_DATA)
                                    fc->auto_inval_data = 1;
    -                       if (arg->flags & FUSE_PRECISE_INVAL_DATA)
    +                       else if (arg->flags & FUSE_PRECISE_INVAL_DATA)
                                    fc->precise_inval_data = 1;
    -                       if (fc->auto_inval_data && fc->precise_inval_data) {
    -                               pr_warn("filesystem requested both auto and "
    -                                       "precise cache control - using auto\n");
    -                               fc->precise_inval_data = 0;
    -                       }
                            if (arg->flags & FUSE_DO_READDIRPLUS) {
                                    fc->do_readdirplus = 1;
                                    if (arg->flags & FUSE_READDIRPLUS_AUTO)

Even though it is ok for me personally (I could be careful and use only
FUSE_PRECISE_INVAL_DATA) I still think usage of both "auto" and "precise"
invalidation modes deserves a warning. It is only at filesystem init time. What
is the reason not to print it?

- "fuse: retrieve: cap requested size to negotiated max_write"

     Signed-off-by: Kirill Smelkov <kirr@xxxxxxxxxx>
     Cc: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
     Cc: Jakob Unterwurzacher <jakobunt@xxxxxxxxx>
    -Cc: <stable@xxxxxxxxxxxxxxx> # v2.6.36+

what is the reason not to include this patch into stable series?


- "fuse: require /dev/fuse reads to have enough buffer capacity"

    --- b/fs/fuse/dev.c
    +++ b/fs/fuse/dev.c
    @@ -1324,7 +1324,7 @@
             * to indicate that it is not following FUSE server/client contract.
             * Don't dequeue / abort any request.
             */
    -       if (nbytes < (fc->conn_init ? 4096 + fc->max_write : FUSE_MIN_READ_BUFFER))
    +       if (nbytes < max_t(size_t, FUSE_MIN_READ_BUFFER, 4096 + fc->max_write))
                    return -EINVAL;

ok, this seems to be correct, since fc, including fc->max_write, is
initialized as all zeros by fuse_conn_init (no amendment needed here).


> > I see. Probably it is not "quoted-printable" as
> >
> >         Content-Type: text/plain; charset=utf-8
> >         Content-Transfer-Encoding: 8bit
>
> Well, google converts it to quoted-printable then:
>
> Content-Type: text/plain; charset=utf-8
> Content-Transfer-Encoding: quoted-printable

I see.

> > suggests and it is maybe due to UTF-8 characters (I used "Â" several
> > times in patch description).
>
> Please refrain from gratuitous use of non-ascii.   That middle-dot is
> written as "*" in C (fixed the patch description).

Ok, I will try not to use unicode for my kernel fuse bits.

Thanks, again, a lot for merging the patches.

Kirill