MIME-Version: 1.0
References: <20221217183142.1425132-1-evanhensbergen@icloud.com>
 <20221218232217.1713283-1-evanhensbergen@icloud.com> <20221218232217.1713283-4-evanhensbergen@icloud.com>
 <Y89F0KGdEBcwu39Y@codewreck.org>
In-Reply-To: <Y89F0KGdEBcwu39Y@codewreck.org>
From:   Eric Van Hensbergen <ericvh@gmail.com>
Date:   Mon, 23 Jan 2023 21:03:12 -0600
Message-ID: <CAFkjPTn9nvFB3=iGaKA7WrXvkQwASqr8G1HvB4GBDZFT-2aGsQ@mail.gmail.com>
Subject: Re: [PATCH v2 03/10] Consolidate file operations and add readahead
 and writeback
To:     asmadeus@codewreck.org
Cc:     Eric Van Hensbergen <evanhensbergen@icloud.com>,
        v9fs-developer@lists.sourceforge.net, rminnich@gmail.com,
        lucho@ionkov.net, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux_oss@crudebyte.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Yeah - the dir_release of a regular file is...interesting.
In any case, IIRC the file_write_and_wait_range is effectively a
cache-flush if we are holding a write-buffer and is required for
getting rid of the writeback_fid so it is actually related to the
cache restructuring.

     -eric


On Mon, Jan 23, 2023 at 8:45 PM <asmadeus@codewreck.org> wrote:
>
> Eric Van Hensbergen wrote on Sun, Dec 18, 2022 at 11:22:13PM +0000:
> > We had 3 different sets of file operations across 2 different protocol
> > variants differentiated by cache which really only changed 3
> > functions.  But the real problem is that certain file modes, mount
> > options, and other factors weren't being considered when we
> > decided whether or not to use caches.
> >
> > This consolidates all the operations and switches
> > to conditionals within a common set to decide whether or not
> > to do different aspects of caching.
> >
> > Signed-off-by: Eric Van Hensbergen <evanhensbergen@icloud.com>
> > ---
> >  fs/9p/v9fs.c           |  30 ++++------
> >  fs/9p/v9fs.h           |   2 +
> >  fs/9p/v9fs_vfs.h       |   4 --
> >  fs/9p/vfs_dir.c        |   9 +++
> >  fs/9p/vfs_file.c       | 123 +++++++----------------------------------
> >  fs/9p/vfs_inode.c      |  31 ++++-------
> >  fs/9p/vfs_inode_dotl.c |  19 ++++++-
> >  7 files changed, 71 insertions(+), 147 deletions(-)
> >
> > diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c
> > index 1675a196c2ba..536769cdf7c8 100644
> > --- a/fs/9p/vfs_dir.c
> > +++ b/fs/9p/vfs_dir.c
> > @@ -214,6 +214,15 @@ int v9fs_dir_release(struct inode *inode, struct file *filp)
> >       p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n",
> >                inode, filp, fid ? fid->fid : -1);
> >       if (fid) {
> > +             if ((fid->qid.type == P9_QTFILE) && (filp->f_mode & FMODE_WRITE)) {
>
> dir release, but the fid is of type regular file ?
>
> Either way this doesn't look directly related to cache level
> consodilations, probably better in another commit.
>
> > +                     int retval = file_write_and_wait_range(filp, 0, -1);
> > +
> > +                     if (retval != 0) {
> > +                             p9_debug(P9_DEBUG_ERROR,
> > +                                     "trying to flush filp %p failed with error code %d\n",
> > +                                     filp, retval);
> > +                     }
> > +             }
> >               spin_lock(&inode->i_lock);
> >               hlist_del(&fid->ilist);
> >               spin_unlock(&inode->i_lock);
> --
> Dominique