On Fri, Dec 20, 2013 at 06:54:40PM +0400, Maxim Patlasov wrote: > Hi Miklos, > > Sorry for delay, see please inline comments below. > > On 11/12/2013 09:17 PM, Miklos Szeredi wrote: > >On Thu, Oct 10, 2013 at 05:11:25PM +0400, Maxim Patlasov wrote: > >>Move the code filling and sending read request to a separate function. > >>Future > >>patches will use it for .write_begin -- partial modification of a page > >>requires reading the page from the storage very similarly to what > >>fuse_readpage > >>does. > >> > >>Signed-off-by: Maxim Patlasov <mpatla...@parallels.com> > >>--- > >> fs/fuse/file.c | 55 > >> +++++++++++++++++++++++++++++++++++++------------------ > >> 1 file changed, 37 insertions(+), 18 deletions(-) > >> > >>diff --git a/fs/fuse/file.c b/fs/fuse/file.c > >>index b4d4189..77eb849 100644 > >>--- a/fs/fuse/file.c > >>+++ b/fs/fuse/file.c > >>@@ -700,21 +700,14 @@ static void fuse_short_read(struct fuse_req *req, > >>struct inode *inode, > >> } > >> } > >>-static int fuse_readpage(struct file *file, struct page *page) > >>+static int __fuse_readpage(struct file *file, struct page *page, size_t > >>count, > >>+ int *err, struct fuse_req **req_pp, u64 *attr_ver_p) > >Signature of this helper looks really ugly. A quick look tells me that > >neither > >caller actually needs 'req'. > > fuse_readpage() passes 'req' to fuse_short_read(). And the latter > uses req->pages[] to nullify a part of request.
I don't get it. __fuse_readpage() itself call's fuse_short_read(), not callers of __fuse_readpage(). Or do they? > > >And fuse_get_attr_version() can be moved to the > >one caller that needs it. > > Yes, it's doable. But this would make attr_version mechanism less > efficient (under some loads): suppose the file on server was > truncated externally, then fuse_readpage() acquires > fc->attr_version, then some innocent write bumps fc->attr_version > while we're waiting for fuse writeback, then fuse_read_update_size() > would noop. In the other words, it's beneficial to keep the time > interval between acquiring fc->attr_version and subsequent > comparison as short as possible. Okay, lets try to keep this the way it is. I don't like it very much, but I fear changing user visible behavior. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
