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@nexedi.com 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@nexedi.com Cc: Han-Wen Nienhuys hanwen@google.com Cc: Jakob Unterwurzacher jakobunt@gmail.com -Cc: stable@vger.kernel.org # 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: 8bitWell, 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