Re: [PATCH v3 20/20] gfs2: clean up some filemap_* calls

On Mon, 2017-04-24 at 13:41 -0400, Bob Peterson wrote:

Hi,

----- Original Message -----

quoted

On Mon, 2017-04-24 at 10:12 -0400, Bob Peterson wrote:

quoted

quoted

+	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.
 

quoted

quoted

quoted

 	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!
-- 
Jeff Layton [off-list ref]