gfs2: Fix xattr fsync

PATCH New Andreas Gruenbacher 2 participants 3 messages Oct 4, 2017 – Oct 24, 2017 GFS2 file system

Share & tools

Andreas Gruenbacher

author maintainer

2017-10-04 18:17 UTC #0 lore raw

PATCH 1/1 gfs2: Fix xattr fsync

Make sure that changing xattrs marks the corresponding inode dirty so that a subsequent fsync will sync those changes to disk. We set I_DIRTY_SYNC as well as I_DIRTY_DATASYNC so that both fsync and fdatasync will sync xattr changes: xattrs can contain information critical to how the data can be accessed, so we don't want fdatasync to skip them.

Signed-off-by Andreas Gruenbacher <[email protected]>

---
 fs/gfs2/xattr.c | 40 ++++++++--------------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

fs/gfs2/xattr.c +8 ~~-32~~

index 36ca70924935..588d4f21a273 100644
@@ -231,7 +231,6 @@ static int ea_dealloc_unstuffed(struct gfs2_inode *ip, struct buffer_head *bh,
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct gfs2_rgrpd *rgd;
 	struct gfs2_holder rg_gh;
-	struct buffer_head *dibh;
 	__be64 *dataptrs;
 	u64 bn = 0;
 	u64 bstart = 0;

@@ -308,13 +307,8 @@ static int ea_dealloc_unstuffed(struct gfs2_inode *ip, struct buffer_head *bh,
 		ea->ea_num_ptrs = 0;
 	}
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (!error) {
-		ip->i_inode.i_ctime = current_time(&ip->i_inode);
-		gfs2_trans_add_meta(ip->i_gl, dibh);
-		gfs2_dinode_out(ip, dibh->b_data);
-		brelse(dibh);
-	}
+	ip->i_inode.i_ctime = current_time(&ip->i_inode);
+	__mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
 	gfs2_trans_end(sdp);

@@ -755,7 +749,6 @@ static int ea_alloc_skeleton(struct gfs2_inode *ip, struct gfs2_ea_request *er,
 			     ea_skeleton_call_t skeleton_call, void *private)
 {
 	struct gfs2_alloc_parms ap = { .target = blks };
-	struct buffer_head *dibh;
 	int error;
 
 	error = gfs2_rindex_update(GFS2_SB(&ip->i_inode));

@@ -780,13 +773,8 @@ static int ea_alloc_skeleton(struct gfs2_inode *ip, struct gfs2_ea_request *er,
 	if (error)
 		goto out_end_trans;
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (!error) {
-		ip->i_inode.i_ctime = current_time(&ip->i_inode);
-		gfs2_trans_add_meta(ip->i_gl, dibh);
-		gfs2_dinode_out(ip, dibh->b_data);
-		brelse(dibh);
-	}
+	ip->i_inode.i_ctime = current_time(&ip->i_inode);
+	__mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
 out_end_trans:
 	gfs2_trans_end(GFS2_SB(&ip->i_inode));

@@ -897,7 +885,6 @@ static int ea_set_simple_noalloc(struct gfs2_inode *ip, struct buffer_head *bh,
 				 struct gfs2_ea_header *ea, struct ea_set *es)
 {
 	struct gfs2_ea_request *er = es->es_er;
-	struct buffer_head *dibh;
 	int error;
 
 	error = gfs2_trans_begin(GFS2_SB(&ip->i_inode), RES_DINODE + 2 * RES_EATTR, 0);

@@ -914,14 +901,9 @@ static int ea_set_simple_noalloc(struct gfs2_inode *ip, struct buffer_head *bh,
 	if (es->es_el)
 		ea_set_remove_stuffed(ip, es->es_el);
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (error)
-		goto out;
 	ip->i_inode.i_ctime = current_time(&ip->i_inode);
-	gfs2_trans_add_meta(ip->i_gl, dibh);
-	gfs2_dinode_out(ip, dibh->b_data);
-	brelse(dibh);
-out:
+	__mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
+
 	gfs2_trans_end(GFS2_SB(&ip->i_inode));
 	return error;
 }

@@ -1117,7 +1099,6 @@ static int ea_remove_stuffed(struct gfs2_inode *ip, struct gfs2_ea_location *el)
 {
 	struct gfs2_ea_header *ea = el->el_ea;
 	struct gfs2_ea_header *prev = el->el_prev;
-	struct buffer_head *dibh;
 	int error;
 
 	error = gfs2_trans_begin(GFS2_SB(&ip->i_inode), RES_DINODE + RES_EATTR, 0);

@@ -1138,13 +1119,8 @@ static int ea_remove_stuffed(struct gfs2_inode *ip, struct gfs2_ea_location *el)
 		ea->ea_type = GFS2_EATYPE_UNUSED;
 	}
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (!error) {
-		ip->i_inode.i_ctime = current_time(&ip->i_inode);
-		gfs2_trans_add_meta(ip->i_gl, dibh);
-		gfs2_dinode_out(ip, dibh->b_data);
-		brelse(dibh);
-	}
+	ip->i_inode.i_ctime = current_time(&ip->i_inode);
+	__mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
 	gfs2_trans_end(GFS2_SB(&ip->i_inode));

Signature · 1 line

2.13.5

Andreas Gruenbacher

author maintainer

2017-10-23 20:58 UTC #1 lore raw

PATCH 1/1 gfs2: Fix xattr fsync

I forgot to mention:

Quoted 6 lines · Andreas Gruenbacher

Make sure that changing xattrs marks the corresponding inode dirty so that a subsequent fsync will sync those changes to disk. We set I_DIRTY_SYNC as well as I_DIRTY_DATASYNC so that both fsync and fdatasync will sync xattr changes: xattrs can contain information critical to how the data can be accessed, so we don't want fdatasync to skip them.

Fixes xfstest generic/066.

Quoted 109 lines

Signed-off-by Andreas Gruenbacher <[email protected]>

---
 fs/gfs2/xattr.c | 40 ++++++++--------------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

fs/gfs2/xattr.c +8 ~~-33~~

index 36ca70924935..588d4f21a273 100644
@@ -231,7 +231,6 @@ static int ea_dealloc_unstuffed(struct gfs2_inode *ip, struct buffer_head *bh,
        struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
        struct gfs2_rgrpd *rgd;
        struct gfs2_holder rg_gh;
-       struct buffer_head *dibh;
        __be64 *dataptrs;
        u64 bn = 0;
        u64 bstart = 0;

@@ -308,13 +307,8 @@ static int ea_dealloc_unstuffed(struct gfs2_inode *ip, struct buffer_head *bh,
                ea->ea_num_ptrs = 0;
        }
 
-       error = gfs2_meta_inode_buffer(ip, &dibh);
-       if (!error) {
-               ip->i_inode.i_ctime = current_time(&ip->i_inode);
-               gfs2_trans_add_meta(ip->i_gl, dibh);
-               gfs2_dinode_out(ip, dibh->b_data);
-               brelse(dibh);
-       }
+       ip->i_inode.i_ctime = current_time(&ip->i_inode);
+       __mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
        gfs2_trans_end(sdp);

@@ -755,7 +749,6 @@ static int ea_alloc_skeleton(struct gfs2_inode *ip, struct gfs2_ea_request *er,
                             ea_skeleton_call_t skeleton_call, void *private)
 {
        struct gfs2_alloc_parms ap = { .target = blks };
-       struct buffer_head *dibh;
        int error;
 
        error = gfs2_rindex_update(GFS2_SB(&ip->i_inode));

@@ -780,13 +773,8 @@ static int ea_alloc_skeleton(struct gfs2_inode *ip, struct gfs2_ea_request *er,
        if (error)
                goto out_end_trans;
 
-       error = gfs2_meta_inode_buffer(ip, &dibh);
-       if (!error) {
-               ip->i_inode.i_ctime = current_time(&ip->i_inode);
-               gfs2_trans_add_meta(ip->i_gl, dibh);
-               gfs2_dinode_out(ip, dibh->b_data);
-               brelse(dibh);
-       }
+       ip->i_inode.i_ctime = current_time(&ip->i_inode);
+       __mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
 out_end_trans:
        gfs2_trans_end(GFS2_SB(&ip->i_inode));

@@ -897,7 +885,6 @@ static int ea_set_simple_noalloc(struct gfs2_inode *ip, struct buffer_head *bh,
                                 struct gfs2_ea_header *ea, struct ea_set *es)
 {
        struct gfs2_ea_request *er = es->es_er;
-       struct buffer_head *dibh;
        int error;
 
        error = gfs2_trans_begin(GFS2_SB(&ip->i_inode), RES_DINODE + 2 * RES_EATTR, 0);

@@ -914,14 +901,9 @@ static int ea_set_simple_noalloc(struct gfs2_inode *ip, struct buffer_head *bh,
        if (es->es_el)
                ea_set_remove_stuffed(ip, es->es_el);
 
-       error = gfs2_meta_inode_buffer(ip, &dibh);
-       if (error)
-               goto out;
        ip->i_inode.i_ctime = current_time(&ip->i_inode);
-       gfs2_trans_add_meta(ip->i_gl, dibh);
-       gfs2_dinode_out(ip, dibh->b_data);
-       brelse(dibh);
-out:
+       __mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
+
        gfs2_trans_end(GFS2_SB(&ip->i_inode));
        return error;
 }

@@ -1117,7 +1099,6 @@ static int ea_remove_stuffed(struct gfs2_inode *ip, struct gfs2_ea_location *el)
 {
        struct gfs2_ea_header *ea = el->el_ea;
        struct gfs2_ea_header *prev = el->el_prev;
-       struct buffer_head *dibh;
        int error;
 
        error = gfs2_trans_begin(GFS2_SB(&ip->i_inode), RES_DINODE + RES_EATTR, 0);

@@ -1138,13 +1119,8 @@ static int ea_remove_stuffed(struct gfs2_inode *ip, struct gfs2_ea_location *el)
                ea->ea_type = GFS2_EATYPE_UNUSED;
        }
 
-       error = gfs2_meta_inode_buffer(ip, &dibh);
-       if (!error) {
-               ip->i_inode.i_ctime = current_time(&ip->i_inode);
-               gfs2_trans_add_meta(ip->i_gl, dibh);
-               gfs2_dinode_out(ip, dibh->b_data);
-               brelse(dibh);
-       }
+       ip->i_inode.i_ctime = current_time(&ip->i_inode);
+       __mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
        gfs2_trans_end(GFS2_SB(&ip->i_inode));
 
--

2.13.5

Andrew Price

2017-10-24 10:46 UTC #2 lore raw

PATCH 1/1 gfs2: Fix xattr fsync

In reply to Andreas Gruenbacher

Quoted 8 lines · Andreas Gruenbacher

Make sure that changing xattrs marks the corresponding inode dirty so that a subsequent fsync will sync those changes to disk. We set I_DIRTY_SYNC as well as I_DIRTY_DATASYNC so that both fsync and fdatasync will sync xattr changes: xattrs can contain information critical to how the data can be accessed, so we don't want fdatasync to skip them.
Signed-off-by Andreas Gruenbacher <[email protected]>

Reviewed-by Andrew Price <[email protected]>

The new __mark_inode_dirty() -> gfs2_dirty_inode() path looks ok in this context.

Andy

Quoted 108 lines

---
 fs/gfs2/xattr.c | 40 ++++++++--------------------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

fs/gfs2/xattr.c +8 ~~-32~~

index 36ca70924935..588d4f21a273 100644
@@ -231,7 +231,6 @@ static int ea_dealloc_unstuffed(struct gfs2_inode *ip, struct buffer_head *bh,
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct gfs2_rgrpd *rgd;
 	struct gfs2_holder rg_gh;
-	struct buffer_head *dibh;
 	__be64 *dataptrs;
 	u64 bn = 0;
 	u64 bstart = 0;

@@ -308,13 +307,8 @@ static int ea_dealloc_unstuffed(struct gfs2_inode *ip, struct buffer_head *bh,
 		ea->ea_num_ptrs = 0;
 	}
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (!error) {
-		ip->i_inode.i_ctime = current_time(&ip->i_inode);
-		gfs2_trans_add_meta(ip->i_gl, dibh);
-		gfs2_dinode_out(ip, dibh->b_data);
-		brelse(dibh);
-	}
+	ip->i_inode.i_ctime = current_time(&ip->i_inode);
+	__mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
 	gfs2_trans_end(sdp);

@@ -755,7 +749,6 @@ static int ea_alloc_skeleton(struct gfs2_inode *ip, struct gfs2_ea_request *er,
 			     ea_skeleton_call_t skeleton_call, void *private)
 {
 	struct gfs2_alloc_parms ap = { .target = blks };
-	struct buffer_head *dibh;
 	int error;
 
 	error = gfs2_rindex_update(GFS2_SB(&ip->i_inode));

@@ -780,13 +773,8 @@ static int ea_alloc_skeleton(struct gfs2_inode *ip, struct gfs2_ea_request *er,
 	if (error)
 		goto out_end_trans;
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (!error) {
-		ip->i_inode.i_ctime = current_time(&ip->i_inode);
-		gfs2_trans_add_meta(ip->i_gl, dibh);
-		gfs2_dinode_out(ip, dibh->b_data);
-		brelse(dibh);
-	}
+	ip->i_inode.i_ctime = current_time(&ip->i_inode);
+	__mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
 out_end_trans:
 	gfs2_trans_end(GFS2_SB(&ip->i_inode));

@@ -897,7 +885,6 @@ static int ea_set_simple_noalloc(struct gfs2_inode *ip, struct buffer_head *bh,
 				 struct gfs2_ea_header *ea, struct ea_set *es)
 {
 	struct gfs2_ea_request *er = es->es_er;
-	struct buffer_head *dibh;
 	int error;
 
 	error = gfs2_trans_begin(GFS2_SB(&ip->i_inode), RES_DINODE + 2 * RES_EATTR, 0);

@@ -914,14 +901,9 @@ static int ea_set_simple_noalloc(struct gfs2_inode *ip, struct buffer_head *bh,
 	if (es->es_el)
 		ea_set_remove_stuffed(ip, es->es_el);
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (error)
-		goto out;
 	ip->i_inode.i_ctime = current_time(&ip->i_inode);
-	gfs2_trans_add_meta(ip->i_gl, dibh);
-	gfs2_dinode_out(ip, dibh->b_data);
-	brelse(dibh);
-out:
+	__mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
+
 	gfs2_trans_end(GFS2_SB(&ip->i_inode));
 	return error;
 }

@@ -1117,7 +1099,6 @@ static int ea_remove_stuffed(struct gfs2_inode *ip, struct gfs2_ea_location *el)
 {
 	struct gfs2_ea_header *ea = el->el_ea;
 	struct gfs2_ea_header *prev = el->el_prev;
-	struct buffer_head *dibh;
 	int error;
 
 	error = gfs2_trans_begin(GFS2_SB(&ip->i_inode), RES_DINODE + RES_EATTR, 0);

@@ -1138,13 +1119,8 @@ static int ea_remove_stuffed(struct gfs2_inode *ip, struct gfs2_ea_location *el)
 		ea->ea_type = GFS2_EATYPE_UNUSED;
 	}
 
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (!error) {
-		ip->i_inode.i_ctime = current_time(&ip->i_inode);
-		gfs2_trans_add_meta(ip->i_gl, dibh);
-		gfs2_dinode_out(ip, dibh->b_data);
-		brelse(dibh);
-	}
+	ip->i_inode.i_ctime = current_time(&ip->i_inode);
+	__mark_inode_dirty(&ip->i_inode, I_DIRTY_SYNC | I_DIRTY_DATASYNC);
 
 	gfs2_trans_end(GFS2_SB(&ip->i_inode));

Signature · 1 line

2.13.5