[v3,20/20] gfs2: clean up some filemap_* calls
diff mbox

Message ID 20170424132259.8680-21-jlayton@redhat.com
State Not Applicable
Headers show

Commit Message

Jeff Layton April 24, 2017, 1:22 p.m. UTC
In some places, it's trying to reset the mapping error after calling
filemap_fdatawait. That's no longer required. Also, turn several
filemap_fdatawrite+filemap_fdatawait calls into filemap_write_and_wait.
That will at least return writeback errors that occur during the write
phase.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/gfs2/glops.c | 12 ++++--------
 fs/gfs2/lops.c  |  4 +---
 fs/gfs2/super.c |  6 ++----
 3 files changed, 7 insertions(+), 15 deletions(-)

Comments

Bob Peterson April 24, 2017, 2:12 p.m. UTC | #1
----- Original Message -----
| In some places, it's trying to reset the mapping error after calling
| filemap_fdatawait. That's no longer required. Also, turn several
| filemap_fdatawrite+filemap_fdatawait calls into filemap_write_and_wait.
| That will at least return writeback errors that occur during the write
| phase.
| 
| Signed-off-by: Jeff Layton <jlayton@redhat.com>
| ---
|  fs/gfs2/glops.c | 12 ++++--------
|  fs/gfs2/lops.c  |  4 +---
|  fs/gfs2/super.c |  6 ++----
|  3 files changed, 7 insertions(+), 15 deletions(-)
| 
| diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
| index 5db59d444838..7362d19fdc4c 100644
| --- a/fs/gfs2/glops.c
| +++ b/fs/gfs2/glops.c
| @@ -158,9 +158,7 @@ static void rgrp_go_sync(struct gfs2_glock *gl)
|  	GLOCK_BUG_ON(gl, gl->gl_state != LM_ST_EXCLUSIVE);
|  
|  	gfs2_log_flush(sdp, gl, NORMAL_FLUSH);
| -	filemap_fdatawrite_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
| -	error = filemap_fdatawait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
| -	mapping_set_error(mapping, error);
| +	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);

This should probably have "error = ", no?

|  	gfs2_ail_empty_gl(gl);
|  
|  	spin_lock(&gl->gl_lockref.lock);
| @@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
|  	filemap_fdatawrite(metamapping);
|  	if (ip) {
|  		struct address_space *mapping = ip->i_inode.i_mapping;
| -		filemap_fdatawrite(mapping);
| -		error = filemap_fdatawait(mapping);
| -		mapping_set_error(mapping, error);
| +		filemap_write_and_wait(mapping);
| +	} else {
| +		filemap_fdatawait(metamapping);
|  	}
| -	error = filemap_fdatawait(metamapping);
| -	mapping_set_error(metamapping, error);

This part doesn't look right at all. There's a big difference in gfs2 between
mapping and metamapping. We need to wait for metamapping regardless.

(snip)

Regards,

Bob Peterson
Red Hat File Systems
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton April 24, 2017, 4:59 p.m. UTC | #2
On Mon, 2017-04-24 at 10:12 -0400, Bob Peterson wrote:
> ----- Original Message -----
> > In some places, it's trying to reset the mapping error after calling
> > filemap_fdatawait. That's no longer required. Also, turn several
> > filemap_fdatawrite+filemap_fdatawait calls into filemap_write_and_wait.
> > That will at least return writeback errors that occur during the write
> > phase.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/gfs2/glops.c | 12 ++++--------
> >  fs/gfs2/lops.c  |  4 +---
> >  fs/gfs2/super.c |  6 ++----
> >  3 files changed, 7 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> > index 5db59d444838..7362d19fdc4c 100644
> > --- a/fs/gfs2/glops.c
> > +++ b/fs/gfs2/glops.c
> > @@ -158,9 +158,7 @@ static void rgrp_go_sync(struct gfs2_glock *gl)
> >  	GLOCK_BUG_ON(gl, gl->gl_state != LM_ST_EXCLUSIVE);
> >  
> >  	gfs2_log_flush(sdp, gl, NORMAL_FLUSH);
> > -	filemap_fdatawrite_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
> > -	error = filemap_fdatawait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
> > -	mapping_set_error(mapping, error);
> > +	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
> 
> This should probably have "error = ", no?
> 

This error is discarded in the current code after resetting the error in
the mapping. With the earlier patches in this set we don't need to reset
the error like this anymore.

Now, if this code should doing something else with those errors, then
that's a separate problem.

> >  	gfs2_ail_empty_gl(gl);
> >  
> >  	spin_lock(&gl->gl_lockref.lock);
> > @@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
> >  	filemap_fdatawrite(metamapping);
> >  	if (ip) {
> >  		struct address_space *mapping = ip->i_inode.i_mapping;
> > -		filemap_fdatawrite(mapping);
> > -		error = filemap_fdatawait(mapping);
> > -		mapping_set_error(mapping, error);
> > +		filemap_write_and_wait(mapping);
> > +	} else {
> > +		filemap_fdatawait(metamapping);
> >  	}
> > -	error = filemap_fdatawait(metamapping);
> > -	mapping_set_error(metamapping, error);
> 
> This part doesn't look right at all. There's a big difference in gfs2 between
> mapping and metamapping. We need to wait for metamapping regardless.
> 

...and this should wait. Basically, filemap_write_and_wait does
filemap_fdatawrite and then filemap_fdatawait. This is mostly just
replacing the existing code with a more concise helper.
Bob Peterson April 24, 2017, 5:41 p.m. UTC | #3
Hi,

----- Original Message -----
| On Mon, 2017-04-24 at 10:12 -0400, Bob Peterson wrote:
| > > +	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
| > 
| > This should probably have "error = ", no?
| > 
| 
| This error is discarded in the current code after resetting the error in
| the mapping. With the earlier patches in this set we don't need to reset
| the error like this anymore.
| 
| Now, if this code should doing something else with those errors, then
| that's a separate problem.

Okay, I see. My bad.
 
| > >  	gfs2_ail_empty_gl(gl);
| > >  
| > >  	spin_lock(&gl->gl_lockref.lock);
| > > @@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
| > >  	filemap_fdatawrite(metamapping);
| > >  	if (ip) {
| > >  		struct address_space *mapping = ip->i_inode.i_mapping;
| > > -		filemap_fdatawrite(mapping);
| > > -		error = filemap_fdatawait(mapping);
| > > -		mapping_set_error(mapping, error);
| > > +		filemap_write_and_wait(mapping);
| > > +	} else {
| > > +		filemap_fdatawait(metamapping);
| > >  	}
| > > -	error = filemap_fdatawait(metamapping);
| > > -	mapping_set_error(metamapping, error);
| > 
| > This part doesn't look right at all. There's a big difference in gfs2
| > between
| > mapping and metamapping. We need to wait for metamapping regardless.
| > 
| 
| ...and this should wait. Basically, filemap_write_and_wait does
| filemap_fdatawrite and then filemap_fdatawait. This is mostly just
| replacing the existing code with a more concise helper.

But this isn't a simple replacement with a helper. This is two different
address spaces (mapping and metamapping) and you added an else in there.

So with this patch metamapping gets written, and if there's an ip,
mapping gets written but it doesn't wait for metamapping. Unless
I'm missing something.

You could replace both filemap_fdatawrites with the helper instead.
Today's code is structured as:

(a) write metamapping
if (ip)
    (b) write mapping
    (c) wait for mapping
(d) wait for metamapping

If you use the helper for both, it becomes, (a & d)(b & c) which is probably
acceptable. (I think we just tried to optimize what the elevator was doing).

But the way you've got it coded here still looks wrong. It looks like:
(a)
if (ip)
   (b & c)
ELSE -
   (d)

So (d) (metamapping) isn't guaranteed to be synced at the end of the function.
Of course, you know the modified helper functions better than I do.
What am I missing?

Regards,

Bob Peterson
Red Hat File Systems
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton April 24, 2017, 5:52 p.m. UTC | #4
On Mon, 2017-04-24 at 13:41 -0400, Bob Peterson wrote:
> Hi,
> 
> ----- Original Message -----
> > On Mon, 2017-04-24 at 10:12 -0400, Bob Peterson wrote:
> > > > +	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
> > > 
> > > This should probably have "error = ", no?
> > > 
> > 
> > This error is discarded in the current code after resetting the error in
> > the mapping. With the earlier patches in this set we don't need to reset
> > the error like this anymore.
> > 
> > Now, if this code should doing something else with those errors, then
> > that's a separate problem.
> 
> Okay, I see. My bad.
>  
> > > >  	gfs2_ail_empty_gl(gl);
> > > >  
> > > >  	spin_lock(&gl->gl_lockref.lock);
> > > > @@ -225,12 +223,10 @@ static void inode_go_sync(struct gfs2_glock *gl)
> > > >  	filemap_fdatawrite(metamapping);
> > > >  	if (ip) {
> > > >  		struct address_space *mapping = ip->i_inode.i_mapping;
> > > > -		filemap_fdatawrite(mapping);
> > > > -		error = filemap_fdatawait(mapping);
> > > > -		mapping_set_error(mapping, error);
> > > > +		filemap_write_and_wait(mapping);
> > > > +	} else {
> > > > +		filemap_fdatawait(metamapping);
> > > >  	}
> > > > -	error = filemap_fdatawait(metamapping);
> > > > -	mapping_set_error(metamapping, error);
> > > 
> > > This part doesn't look right at all. There's a big difference in gfs2
> > > between
> > > mapping and metamapping. We need to wait for metamapping regardless.
> > > 
> > 
> > ...and this should wait. Basically, filemap_write_and_wait does
> > filemap_fdatawrite and then filemap_fdatawait. This is mostly just
> > replacing the existing code with a more concise helper.
> 
> But this isn't a simple replacement with a helper. This is two different
> address spaces (mapping and metamapping) and you added an else in there.
> 
> So with this patch metamapping gets written, and if there's an ip,
> mapping gets written but it doesn't wait for metamapping. Unless
> I'm missing something.
> 
> You could replace both filemap_fdatawrites with the helper instead.
> Today's code is structured as:
> 
> (a) write metamapping
> if (ip)
>     (b) write mapping
>     (c) wait for mapping
> (d) wait for metamapping
> 
> If you use the helper for both, it becomes, (a & d)(b & c) which is probably
> acceptable. (I think we just tried to optimize what the elevator was doing).
> 
> But the way you've got it coded here still looks wrong. It looks like:
> (a)
> if (ip)
>    (b & c)
> ELSE -
>    (d)
> 
> So (d) (metamapping) isn't guaranteed to be synced at the end of the function.
> Of course, you know the modified helper functions better than I do.
> What am I missing?
> 
> 

<facepalm>
You're right of course. I'll fix that up in my tree.

Thanks!

Patch
diff mbox

diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 5db59d444838..7362d19fdc4c 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -158,9 +158,7 @@  static void rgrp_go_sync(struct gfs2_glock *gl)
 	GLOCK_BUG_ON(gl, gl->gl_state != LM_ST_EXCLUSIVE);
 
 	gfs2_log_flush(sdp, gl, NORMAL_FLUSH);
-	filemap_fdatawrite_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
-	error = filemap_fdatawait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
-	mapping_set_error(mapping, error);
+	filemap_write_and_wait_range(mapping, gl->gl_vm.start, gl->gl_vm.end);
 	gfs2_ail_empty_gl(gl);
 
 	spin_lock(&gl->gl_lockref.lock);
@@ -225,12 +223,10 @@  static void inode_go_sync(struct gfs2_glock *gl)
 	filemap_fdatawrite(metamapping);
 	if (ip) {
 		struct address_space *mapping = ip->i_inode.i_mapping;
-		filemap_fdatawrite(mapping);
-		error = filemap_fdatawait(mapping);
-		mapping_set_error(mapping, error);
+		filemap_write_and_wait(mapping);
+	} else {
+		filemap_fdatawait(metamapping);
 	}
-	error = filemap_fdatawait(metamapping);
-	mapping_set_error(metamapping, error);
 	gfs2_ail_empty_gl(gl);
 	/*
 	 * Writeback of the data mapping may cause the dirty flag to be set
diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index b1f9144b42c7..565ce33dec9b 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -586,9 +586,7 @@  static void gfs2_meta_sync(struct gfs2_glock *gl)
 	if (mapping == NULL)
 		mapping = &sdp->sd_aspace;
 
-	filemap_fdatawrite(mapping);
-	error = filemap_fdatawait(mapping);
-
+	error = filemap_write_and_wait(mapping);
 	if (error)
 		gfs2_io_error(gl->gl_name.ln_sbd);
 }
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 361796a84fce..675c39566ea1 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1593,10 +1593,8 @@  static void gfs2_evict_inode(struct inode *inode)
 out_truncate:
 	gfs2_log_flush(sdp, ip->i_gl, NORMAL_FLUSH);
 	metamapping = gfs2_glock2aspace(ip->i_gl);
-	if (test_bit(GLF_DIRTY, &ip->i_gl->gl_flags)) {
-		filemap_fdatawrite(metamapping);
-		filemap_fdatawait(metamapping);
-	}
+	if (test_bit(GLF_DIRTY, &ip->i_gl->gl_flags))
+		filemap_write_and_wait(metamapping);
 	write_inode_now(inode, 1);
 	gfs2_ail_flush(ip->i_gl, 0);