| Message ID | 20170424132259.8680-21-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
----- 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
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.
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
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!
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);
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(-)