GFS2: Fix allocation error bug with recursive rgrp glocking

PATCH New Bob Peterson 2 participants 3 messages May 30, 2018 – May 31, 2018 1 message not in the archive GFS2 file system

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bob Peterson

author

2018-05-30 19:14 UTC #1 lore raw

PATCH 1/1 GFS2: Fix allocation error bug with recursive rgrp glocking

Hi,

Before this patch function gfs2_write_begin, upon discovering an error, called gfs2_trim_blocks while the rgrp glock was still held. That's because gfs2_inplace_release is not called until later. This patch reorganizes the logic a bit so gfs2_inplace_release is called to release the lock prior to the call to gfs2_trim_blocks, thus preventing the glock recursion.

Signed-off-by Bob Peterson <[email protected]>

---
 fs/gfs2/aops.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

fs/gfs2/aops.c +2 -5

index f58716567972..629e614b324c 100644
@@ -746,16 +746,13 @@ static int gfs2_write_begin(struct file *file, struct address_space *mapping,
 	unlock_page(page);
 	put_page(page);
 
-	gfs2_trans_end(sdp);
-	if (pos + len > ip->i_inode.i_size)
-		gfs2_trim_blocks(&ip->i_inode);
-	goto out_trans_fail;
-
 out_endtrans:
 	gfs2_trans_end(sdp);
 out_trans_fail:
 	if (alloc_required) {
 		gfs2_inplace_release(ip);
+		if (pos + len > ip->i_inode.i_size)
+			gfs2_trim_blocks(&ip->i_inode);
 out_qunlock:
 		gfs2_quota_unlock(ip);
 	}

Andreas Gruenbacher

maintainer

2018-05-31 11:08 UTC #2 lore raw

PATCH 1/1 GFS2: Fix allocation error bug with recursive rgrp glocking

Bob,

Quoted 3 lines · Bob Peterson

Before this patch function gfs2_write_begin, upon discovering an error, called gfs2_trim_blocks while the rgrp glock was still held. That's because gfs2_inplace_release is not called until later.

agreed.

Quoted 3 lines

This patch reorganizes the logic a bit so gfs2_inplace_release is called to release the lock prior to the call to gfs2_trim_blocks, thus preventing the glock recursion.

gfs2_trim_blocks only needs to be called after __block_write_begin but with your proposed patch, it also ends up being called on other error paths. This complicates gfs2_write_begin, so how about the below patch instead?

Thanks,
Andreas

---
 fs/gfs2/aops.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

fs/gfs2/aops.c +8 -5

index f58716567972..66e7172e0134 100644
@@ -747,18 +747,21 @@ static int gfs2_write_begin(struct file *file, struct address_space *mapping,
 	put_page(page);
 
 	gfs2_trans_end(sdp);
-	if (pos + len > ip->i_inode.i_size)
-		gfs2_trim_blocks(&ip->i_inode);
-	goto out_trans_fail;
+	if (alloc_required) {
+		gfs2_inplace_release(ip);
+		if (pos + len > ip->i_inode.i_size)
+			gfs2_trim_blocks(&ip->i_inode);
+	}
+	goto out_qunlock;
 
 out_endtrans:
 	gfs2_trans_end(sdp);
 out_trans_fail:
-	if (alloc_required) {
+	if (alloc_required)
 		gfs2_inplace_release(ip);
 out_qunlock:
+	if (alloc_required)
 		gfs2_quota_unlock(ip);
-	}
 out_unlock:
 	if (&ip->i_inode == sdp->sd_rindex) {
 		gfs2_glock_dq(&m_ip->i_gh);

Signature · 1 line

2.17.0

Bob Peterson

author

2018-05-31 12:26 UTC #3 lore raw

PATCH 1/1 GFS2: Fix allocation error bug with recursive rgrp glocking

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

Quoted 13 lines

Bob,
Quoted 3 lines · Bob Peterson
Before this patch function gfs2_write_begin, upon discovering an error, called gfs2_trim_blocks while the rgrp glock was still held. That's because gfs2_inplace_release is not called until later.
agreed.
Quoted 3 lines
This patch reorganizes the logic a bit so gfs2_inplace_release is called to release the lock prior to the call to gfs2_trim_blocks, thus preventing the glock recursion.

Quoted 7 lines

gfs2_trim_blocks only needs to be called after __block_write_begin but with your proposed patch, it also ends up being called on other error paths. This complicates gfs2_write_begin, so how about the below patch instead?
Thanks,
Andreas

Hi Andreas,

My previous version looked a lot like yours, so yes, I'll take your version instead. It's going to be overwritten by your iomap-write patch set eventually anyway.

Regards,

Bob Peterson