Re: [PATCH v4 03/11] fs/9p: Consolidate file operations and add readahead and writeback

[Eric Van Hensbergen]

...of course, relooking at the functions in mm/filemap.c it seems like
I can probably just use filemap_fdatawrite
instead of having my own flush function since it basically sets up wbc
the same way....

On Sat, Feb 18, 2023 at 10:17 AM Eric Van Hensbergen <ericvh@xxxxxxxxx> wrote:
>
> On Sat, Feb 18, 2023 at 3:25 AM <asmadeus@xxxxxxxxxxxxx> wrote:
> >
> > Ok so this bugged me to no end; that seems to be because we use the same
> > v9fs_dir_release for v9fs_file_operations's .release and not just
> > v9fs_dir_operations... So it's to be expected we'll get files here.
> >
> > At this point I'd suggest to use two functions, but that's probably
> > overdoing it.
> > Let's check S_ISREG(inode->i_mode) instead of fid->qid though; it
> > shouldn't make any difference but that's what you use in other parts of
> > the code and it will be easier to understand for people familiar with
> > the vfs.
> >
>
> I can rename the function as part of the patch since it would be a bit
> more accurate,
> but then it is still in vfs_dir.  I think there did used to be two
> functions but there
> was so much overlap we collapsed into one.
>
> >
> > > diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> > > index 33e521c60e2c..8ffa6631b1fd 100644
> > > --- a/fs/9p/vfs_inode.c
> > > +++ b/fs/9p/vfs_inode.c
> > > @@ -219,6 +219,35 @@ v9fs_blank_wstat(struct p9_wstat *wstat)
> > >       wstat->extension = NULL;
> > >  }
> > >
> > > +/**
> > > + * v9fs_flush_inode_writeback - writeback any data associated with inode
> > > + * @inode: inode to writeback
> > > + *
> > > + * This is used to make sure anything that needs to be written
> > > + * to server gets flushed before we do certain operations (setattr, getattr, close)
> > > + *
> > > + */
> > > +
> > > +int v9fs_flush_inode_writeback(struct inode *inode)
> > > +{
> > > +     struct writeback_control wbc = {
> > > +             .nr_to_write = LONG_MAX,
> > > +             .sync_mode = WB_SYNC_ALL,
> > > +             .range_start = 0,
> > > +             .range_end = -1,
> > > +     };
> > > +
> > > +     int retval = filemap_fdatawrite_wbc(inode->i_mapping, &wbc);
> >
> > Hmm, that function only starts the writeback, but doesn't wait for it.
> >
> > Wasn't the point to replace 'filemap_write_and_wait' with
> > v9fs_flush_inode_writeback?
> > I don't think it's a good idea to remove the wait before setattrs and
> > the like; if you don't want to wait on close()'s release (but we
> > probably should too) perhaps split this in two?
> >
>
> I had thought that this is what it does, of course I could just be getting
> lucky.  The filemap_fdatawrite_wbc doesn't say anything about whether
> WBC_SYNC_ALL forces a wait, but the next function (__filemap_fdatawrite_range)
> does: (it it calls filemap_fdatawrite_wbc)
>
> * If sync_mode is WB_SYNC_ALL then this is a "data integrity" operation, as
> * opposed to a regular memory cleansing writeback. The difference between
> * these two operations is that if a dirty page/buffer is encountered, it must
> * be waited upon, and not just skipped over.
>
> So I think we are good?  Happy to use a different function if it makes sense,
> but this was the one that seemed to trigger the correct behavior.
>
>        -eric