Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 9 Jan 2023 15:07:44 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Jeff Layton <jlayton@redhat.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 08/11] cifs: Remove call to filemap_check_wb_err()
Message-ID: <Y7wtwD/Yr+nWSqpx@casper.infradead.org>
References: <20230109051823.480289-1-willy@infradead.org>
 <20230109051823.480289-9-willy@infradead.org>
 <7d1499fadf42052711e39f0d8c7656f4d3a4bc9d.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7d1499fadf42052711e39f0d8c7656f4d3a4bc9d.camel@redhat.com>
Precedence: bulk

On Mon, Jan 09, 2023 at 09:42:36AM -0500, Jeff Layton wrote:
> On Mon, 2023-01-09 at 05:18 +0000, Matthew Wilcox (Oracle) wrote:
> > filemap_write_and_wait() now calls filemap_check_wb_err(), so we cannot
> > glean any additional information by calling it ourselves.  It may also
> > be misleading as it will pick up on any errors since the beginning of
> > time which may well be since before this program opened the file.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  fs/cifs/file.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 22dfc1f8b4f1..7e7ee26cf77d 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -3042,14 +3042,12 @@ int cifs_flush(struct file *file, fl_owner_t id)
> >  	int rc = 0;
> >  
> >  	if (file->f_mode & FMODE_WRITE)
> > -		rc = filemap_write_and_wait(inode->i_mapping);
> > +		rc = filemap_write_and_wait(file->f_mapping);
> 
> If we're calling ->flush, then the file is being closed. Should this
> just be?
> 		rc = file_write_and_wait(file);
> 
> It's not like we need to worry about corrupting ->f_wb_err at that
> point.

Yes, I think you're right, and then this is a standalone patch that can
go in this cycle, perhaps.