Fourteen patches related to file unlink->delete->new
v3 cover Fourteen patches related to file unlink->delete->new
On September 10, I posted a set of 11 preliminary patches for GFS2 that revised the way GFS2 transitions files from unlinked to deleted. I received no feedback. Since that time, I've found several bugs in testing and fixed them. I've also split the last patch into five pieces. This is the revised set of fourteen patches.
On October 6, I posted a revised (v2) set of 15 patches, but as Andrew Elble found out, they weren't quite right. There was still a glock referencing problem. Based on that problem, I eliminated the patch (previously called "gfs2: Use new variable i_gl instead of ip->i_gl") and revised the last one ("GFS2: Rework gfs2_evict_inode to prevent collisions with openers"). This version seems to work better and doesn't leave millions of glocks behind.
This set of fourteen patches was designed to fix several problems in GFS2 that had to do with transitioning dinodes from unlinked to deleted and back to created again. Many of these problems resulted in dinodes being not transitioning from the "unlinked" state to a "deleted" state. Some of them address the problem of dinodes being re-created in place (due to limited space in the file system) in which case callbacks were received and delete work was queued for dinodes that were already deleted and re-used by another node, only to have the callback destroy the newly created dinode.
These patches were tested using virts that had 4GB GFS2 file systems, using a test case that recreated numerous problems. With these patches, all those problems disappear.
---
Bob Peterson (14):
GFS2: Update master statfs buffer with sd_statfs_spin locked
GFS2: Allow fail_gunlock3 to set the free_vfs_inode bit
GFS2: Protect log tail calculations with inside locks
GFS2: Wait for iopen glock dequeues
GFS2: Reintroduce a timeout in function gfs2_gl_hash_clear
GFS2: Prevent gl_delete work for re-used inodes
GFS2: Truncate address space mapping when deleting an inode
GFS2: Don't filter out I_FREEING inodes anymore
GFS2: generalize gfs2_check_blk_type
GFS2: Change from tr_touched to tr_bufs
GFS2: Add new function gfs2_inode_lookup_for_del
gfs2: Remove unused param non_block from gfs2_inode_lookup
GFS2: Hold onto iopen glock longer when dinode creation fails
GFS2: Rework gfs2_evict_inode to prevent collisions with openers
fs/gfs2/dir.c | 2 +- fs/gfs2/export.c | 2 +- fs/gfs2/glock.c | 17 ++-- fs/gfs2/glops.c | 3 +- fs/gfs2/incore.h | 7 +- fs/gfs2/inode.c | 246 ++++++++++++++++++++++++++++++++++++--------------- fs/gfs2/inode.h | 7 +- fs/gfs2/log.c | 7 +- fs/gfs2/main.c | 2 + fs/gfs2/meta_io.c | 12 ++- fs/gfs2/ops_fstype.c | 2 +- fs/gfs2/rgrp.c | 52 ++++++++--- fs/gfs2/rgrp.h | 1 + fs/gfs2/super.c | 52 ++++++++--- fs/gfs2/trans.c | 13 +-- 15 files changed, 300 insertions(+), 125 deletions(-)
Signature · 1 line
2.4.3
PATCH v3 1/14 GFS2: Update master statfs buffer with sd_statfs_spin locked
Before this patch, function update_statfs called gfs2_statfs_change_out to update the master statfs buffer without the sd_statfs_spin held. In theory, another process could call gfs2_statfs_sync, which takes the sd_statfs_spin lock and re-reads m_sc from the buffer. So there's a theoretical timing window in which one process could write the master statfs buffer, then another comes along and re-reads it, wiping out the changes.
--- fs/gfs2/super.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
fs/gfs2/super.c +2 -3
@@ -556,6 +556,7 @@ void update_statfs(struct gfs2_sbd *sdp, struct buffer_head *m_bh,struct gfs2_statfs_change_host *l_sc = &sdp->sd_statfs_local;gfs2_trans_add_meta (l_ip->i_gl, l_bh); +gfs2_trans_add_meta (m_ip->i_gl, m_bh);spin_lock (&sdp->sd_statfs_spin); m_sc->sc_total += l_sc->sc_total;
@@ -564,10 +565,8 @@ void update_statfs(struct gfs2_sbd *sdp, struct buffer_head *m_bh,memset (l_sc,0 ,sizeof (struct gfs2_statfs_change ));memset (l_bh->b_data +sizeof (struct gfs2_dinode ),0 ,sizeof (struct gfs2_statfs_change )); -spin_unlock (&sdp->sd_statfs_spin); - -gfs2_trans_add_meta (m_ip->i_gl, m_bh);gfs2_statfs_change_out (m_sc, m_bh->b_data +sizeof (struct gfs2_dinode )); +spin_unlock (&sdp->sd_statfs_spin); }int gfs2_statfs_sync (struct super_block *sb,int type)
Signature · 1 line
2.4.3
PATCH v3 1/14 GFS2: Update master statfs buffer with sd_statfs_spin locked
Hi,
Looks good to me:
Steve.
Quoted 37 lines · Bob Peterson
Before this patch, function
update_statfscalledgfs2_statfs_change_outto update the master statfs buffer without thesd_statfs_spinheld. In theory, another process could callgfs2_statfs_sync, which takes thesd_statfs_spinlock and re-readsm_scfrom the buffer. So there's a theoretical timing window in which one process could write the master statfs buffer, then another comes along and re-reads it, wiping out the changes.Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/super.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)fs/gfs2/super.c +2
-3@@ -556,6 +556,7 @@ void update_statfs(struct gfs2_sbd *sdp, struct buffer_head *m_bh,struct gfs2_statfs_change_host *l_sc = &sdp->sd_statfs_local;gfs2_trans_add_meta (l_ip->i_gl, l_bh); +gfs2_trans_add_meta (m_ip->i_gl, m_bh);spin_lock (&sdp->sd_statfs_spin); m_sc->sc_total += l_sc->sc_total;@@ -564,10 +565,8 @@ void update_statfs(struct gfs2_sbd *sdp, struct buffer_head *m_bh,memset (l_sc,0 ,sizeof (struct gfs2_statfs_change ));memset (l_bh->b_data +sizeof (struct gfs2_dinode ),0 ,sizeof (struct gfs2_statfs_change )); -spin_unlock (&sdp->sd_statfs_spin); - -gfs2_trans_add_meta (m_ip->i_gl, m_bh);gfs2_statfs_change_out (m_sc, m_bh->b_data +sizeof (struct gfs2_dinode )); +spin_unlock (&sdp->sd_statfs_spin); }int gfs2_statfs_sync (struct super_block *sb,int type)
PATCH v3 2/14 GFS2: Allow fail_gunlock3 to set the free_vfs_inode bit
In reply to Bob PetersonThis patch changes a failure case in function gfs2_create_inode. In some error paths, it jumps to label fail_gunlock3, which fails to set the free_vfs_inode flag. This, in turn, prevents the code from setting the GIF_FREE_VFS_INODE inode flag. That, in turn, allow the code to mistakenly unlink the dinode and not ever delete its data blocks.
--- fs/gfs2/inode.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
fs/gfs2/inode.c +2 -5
@@ -766,11 +766,8 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,return error; fail_gunlock3: -gfs2_glock_dq_uninit (ghs +1 ); -if (ip->i_gl) -gfs2_glock_put (ip->i_gl); - goto fail_gunlock; - +if (ip->i_iopen_gh.gh_gl)/* if holder is linked to the glock */ +gfs2_glock_put (ip->i_iopen_gh.gh_gl); fail_gunlock2:gfs2_glock_dq_uninit (ghs +1 ); fail_free_inode:
Signature · 1 line
2.4.3
PATCH v3 2/14 GFS2: Allow fail_gunlock3 to set the free_vfs_inode bit
Hi,
Quoted 6 lines · Bob Peterson
This patch changes a failure case in function
gfs2_create_inode. In some error paths, it jumps to labelfail_gunlock3, which fails to set thefree_vfs_inodeflag. This, in turn, prevents the code from setting theGIF_FREE_VFS_INODEinode flag. That, in turn, allow the code to mistakenly unlink the dinode and not ever delete its data blocks.
This code path is rather a mess unfortunately. I'm trying to figure out how the inode could have any data blocks at this stage? There should only be the inode itself, and possibly a preallocated block waiting to take xattrs at the time that the fail_gunlock3 path is taken,
Steve.
Quoted 23 lines
Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/inode.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)fs/gfs2/inode.c +2
-5@@ -766,11 +766,8 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,return error; fail_gunlock3: -gfs2_glock_dq_uninit (ghs +1 ); -if (ip->i_gl) -gfs2_glock_put (ip->i_gl); - goto fail_gunlock; - +if (ip->i_iopen_gh.gh_gl)/* if holder is linked to the glock */ +gfs2_glock_put (ip->i_iopen_gh.gh_gl); fail_gunlock2:gfs2_glock_dq_uninit (ghs +1 ); fail_free_inode:
PATCH v3 3/14 GFS2: Protect log tail calculations with inside locks
In reply to Bob PetersonThis patch moves the variables "old_tail" and "wrap" in function gfs2_ail2_empty_one inside the protective spin_lock so they can't be moved by another process. If the calculations aren't protected, the log's tail may not be properly pulled, which eventually makes it seem like there isn't enough room in the journal for more transactions. This results in a hang where GFS2 repeatedly does log flushes without making any progress.
--- fs/gfs2/log.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
fs/gfs2/log.c +4 -3
@@ -277,11 +277,12 @@ static void gfs2_ail2_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr)static void ail2_empty (struct gfs2_sbd *sdp,unsigned int new_tail) {struct gfs2_trans *tr, *safe; -unsigned int old_tail = sdp->sd_log_tail; -int wrap = (new_tail < old_tail); -int a, b, rm; +unsigned int old_tail; +int wrap, a, b, rm;spin_lock (&sdp->sd_ail_lock); + old_tail = sdp->sd_log_tail; + wrap = (new_tail < old_tail);list_for_each_entry_safe (tr, safe, &sdp->sd_ail2_list, tr_list) { a = (old_tail <= tr->tr_first);
Signature · 1 line
2.4.3
PATCH v3 3/14 GFS2: Protect log tail calculations with inside locks
Hi,
sd_log_tail should be protected by sdp->sd_log_flush_lock, so is that not happening for some reason?
Steve.
Quoted 33 lines · Bob Peterson
This patch moves the variables "
old_tail" and "wrap" in functiongfs2_ail2_empty_oneinside the protectivespin_lockso they can't be moved by another process. If the calculations aren't protected, the log's tail may not be properly pulled, which eventually makes it seem like there isn't enough room in the journal for more transactions. This results in a hang where GFS2 repeatedly does log flushes without making any progress.Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/log.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)fs/gfs2/log.c +4
-3@@ -277,11 +277,12 @@ static void gfs2_ail2_empty_one(struct gfs2_sbd *sdp, struct gfs2_trans *tr)static void ail2_empty (struct gfs2_sbd *sdp,unsigned int new_tail) {struct gfs2_trans *tr, *safe; -unsigned int old_tail = sdp->sd_log_tail; -int wrap = (new_tail < old_tail); -int a, b, rm; +unsigned int old_tail; +int wrap, a, b, rm;spin_lock (&sdp->sd_ail_lock); + old_tail = sdp->sd_log_tail; + wrap = (new_tail < old_tail);list_for_each_entry_safe (tr, safe, &sdp->sd_ail2_list, tr_list) { a = (old_tail <= tr->tr_first);
PATCH v3 4/14 GFS2: Wait for iopen glock dequeues
In reply to Bob PetersonThis patch changes every glock_dq for iopen glocks into a dq_wait. This makes sure that iopen glocks do not outlive the inode itself. In turn, that ensures that anyone trying to unlink the glock will be able to find the inode when it receives a remote iopen callback.
--- fs/gfs2/inode.c | 3 ++- fs/gfs2/super.c | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-)
fs/gfs2/inode.c +2 -1
@@ -191,7 +191,8 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type, fail_refresh: ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; ip->i_iopen_gh.gh_gl->gl_object =NULL ; -gfs2_glock_dq_uninit (&ip->i_iopen_gh); +gfs2_glock_dq_wait (&ip->i_iopen_gh); +gfs2_holder_uninit (&ip->i_iopen_gh); fail_iopen:if (io_gl)gfs2_glock_put (io_gl);
fs/gfs2/super.c +5 -3
@@ -1525,7 +1525,8 @@ static void gfs2_evict_inode(struct inode *inode) error =gfs2_glock_nq_init (ip->i_gl,LM_ST_EXCLUSIVE ,GL_SKIP , &gh);if (unlikely (error)) { ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; -gfs2_glock_dq_uninit (&ip->i_iopen_gh); +gfs2_glock_dq_wait (&ip->i_iopen_gh); +gfs2_holder_uninit (&ip->i_iopen_gh); goto out; }
@@ -1597,7 +1598,7 @@ out_unlock:if (test_bit (HIF_HOLDER , &ip->i_iopen_gh.gh_iflags)) { ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; -gfs2_glock_dq (&ip->i_iopen_gh); +gfs2_glock_dq_wait (&ip->i_iopen_gh); }gfs2_holder_uninit (&ip->i_iopen_gh);gfs2_glock_dq_uninit (&gh);
@@ -1618,7 +1619,8 @@ out:if (ip->i_iopen_gh.gh_gl) { ip->i_iopen_gh.gh_gl->gl_object =NULL ; ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; -gfs2_glock_dq_uninit (&ip->i_iopen_gh); +gfs2_glock_dq_wait (&ip->i_iopen_gh); +gfs2_holder_uninit (&ip->i_iopen_gh); } }
Signature · 1 line
2.4.3
PATCH v3 4/14 GFS2: Wait for iopen glock dequeues
Hi,
Steve.
Quoted 58 lines · Bob Peterson
This patch changes every
glock_dqfor iopen glocks into adq_wait. This makes sure that iopen glocks do not outlive the inode itself. In turn, that ensures that anyone trying to unlink the glock will be able to find the inode when it receives a remote iopen callback.Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/inode.c | 3 ++- fs/gfs2/super.c | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-)fs/gfs2/inode.c +2
-1@@ -191,7 +191,8 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type, fail_refresh: ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; ip->i_iopen_gh.gh_gl->gl_object =NULL ; -gfs2_glock_dq_uninit (&ip->i_iopen_gh); +gfs2_glock_dq_wait (&ip->i_iopen_gh); +gfs2_holder_uninit (&ip->i_iopen_gh); fail_iopen:if (io_gl)gfs2_glock_put (io_gl);fs/gfs2/super.c +5
-3@@ -1525,7 +1525,8 @@ static void gfs2_evict_inode(struct inode *inode) error =gfs2_glock_nq_init (ip->i_gl,LM_ST_EXCLUSIVE ,GL_SKIP , &gh);if (unlikely (error)) { ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; -gfs2_glock_dq_uninit (&ip->i_iopen_gh); +gfs2_glock_dq_wait (&ip->i_iopen_gh); +gfs2_holder_uninit (&ip->i_iopen_gh); goto out; }@@ -1597,7 +1598,7 @@ out_unlock:if (test_bit (HIF_HOLDER , &ip->i_iopen_gh.gh_iflags)) { ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; -gfs2_glock_dq (&ip->i_iopen_gh); +gfs2_glock_dq_wait (&ip->i_iopen_gh); }gfs2_holder_uninit (&ip->i_iopen_gh);gfs2_glock_dq_uninit (&gh);@@ -1618,7 +1619,8 @@ out:if (ip->i_iopen_gh.gh_gl) { ip->i_iopen_gh.gh_gl->gl_object =NULL ; ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; -gfs2_glock_dq_uninit (&ip->i_iopen_gh); +gfs2_glock_dq_wait (&ip->i_iopen_gh); +gfs2_holder_uninit (&ip->i_iopen_gh); } }
PATCH v3 5/14 GFS2: Reintroduce a timeout in function gfs2_gl_hash_clear
In reply to Bob PetersonAt some point in the past, we used to have a timeout when GFS2 was unmounting, trying to clear out its glocks. If the timeout expires, it would dump the remaining glocks to the kernel messages so that developers can debug the problem. That timeout was eliminated, probably by accident. This patch reintroduces it.
--- fs/gfs2/glock.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
fs/gfs2/glock.c +3 -1
@@ -1506,7 +1506,9 @@ void gfs2_gl_hash_clear(struct gfs2_sbd *sdp)flush_workqueue (glock_workqueue);glock_hash_walk (clear_glock, sdp);flush_workqueue (glock_workqueue); -wait_event (sdp->sd_glock_wait,atomic_read (&sdp->sd_glock_disposal) ==0 ); +wait_event_timeout (sdp->sd_glock_wait, +atomic_read (&sdp->sd_glock_disposal) ==0 , +HZ *600 );glock_hash_walk (dump_glock_func, sdp); }
Signature · 1 line
2.4.3
PATCH v3 5/14 GFS2: Reintroduce a timeout in function gfs2_gl_hash_clear
Hi,
Steve.
Quoted 26 lines · Bob Peterson
At some point in the past, we used to have a timeout when GFS2 was unmounting, trying to clear out its glocks. If the timeout expires, it would dump the remaining glocks to the kernel messages so that developers can debug the problem. That timeout was eliminated, probably by accident. This patch reintroduces it.
Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/glock.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)fs/gfs2/glock.c +3
-1@@ -1506,7 +1506,9 @@ void gfs2_gl_hash_clear(struct gfs2_sbd *sdp)flush_workqueue (glock_workqueue);glock_hash_walk (clear_glock, sdp);flush_workqueue (glock_workqueue); -wait_event (sdp->sd_glock_wait,atomic_read (&sdp->sd_glock_disposal) ==0 ); +wait_event_timeout (sdp->sd_glock_wait, +atomic_read (&sdp->sd_glock_disposal) ==0 , +HZ *600 );glock_hash_walk (dump_glock_func, sdp); }
PATCH v3 6/14 GFS2: Prevent gl_delete work for re-used inodes
In reply to Bob PetersonThis patch adds a new glock flag GLF_INODE_DELETING which signifies when a glock is being used to change an inode from unlinked to deleted. The flag is used in a few places:
1. If an iopen callback is received, it checks the flag. If the bit is set, someone else has already started deleting the inode. In that case, the delete_func may already be running, so we don't want to queue it to run another time. Doing so only gets us into trouble.
2. When a dinode is in the process of being created, we've been assigned that block by the allocator, so it must have been free. At that point, we check if there is pending delete work pending, and if so, we cancel it to prevent the block from being deleted while we're creating it. This is necessary because there could be pending delete work that was queued up a while ago, but the delete work might have been done on another node, which is how the block became freed. However, we keep the GLF_INODE_DELETING set to prevent new delete work from being queued. After we're done creating, we clear the bit, otherwise the file may not be deleted ever again, even in legitimate cases in the future.
3. In function try_rgrp_unlink, we also make sure the bit isn't already set before we try to reclaim an unlinked block.
--- fs/gfs2/glock.c | 2 ++ fs/gfs2/glops.c | 3 ++- fs/gfs2/incore.h | 1 + fs/gfs2/inode.c | 21 ++++++++++++++++++++- fs/gfs2/rgrp.c | 6 ++++++ 5 files changed, 31 insertions(+), 2 deletions(-)
fs/gfs2/glock.c +2 -0
@@ -1628,6 +1628,8 @@ static const char *gflags2str(char *buf, const struct gfs2_glock *gl) *p++ ='o' ;if (test_bit (GLF_BLOCKING , gflags)) *p++ ='b' ; +if (test_bit (GLF_INODE_DELETING , gflags)) + *p++ ='x' ; *p =0 ;return buf; }
fs/gfs2/glops.c +2 -1
@@ -553,7 +553,8 @@ static void iopen_go_callback(struct gfs2_glock *gl, bool remote)return ;if (gl->gl_demote_state ==LM_ST_UNLOCKED && - gl->gl_state ==LM_ST_SHARED && ip) { + gl->gl_state ==LM_ST_SHARED && ip && + !test_and_set_bit (GLF_INODE_DELETING , &gl->gl_flags)) { gl->gl_lockref.count++;if (queue_work (gfs2_delete_workqueue, &gl->gl_delete) ==0 ) gl->gl_lockref.count--;
fs/gfs2/incore.h +1 -0
@@ -326,6 +326,7 @@ enum {GLF_LRU =13 ,GLF_OBJECT =14 ,/* Used only for tracing */ GLF_BLOCKING =15 , + GLF_INODE_DELETING = 16, /* Was unlinked, being deleted */ }; struct gfs2_glock {
fs/gfs2/inode.c +20 -1
@@ -597,6 +597,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,int error, free_vfs_inode =0 ;u32 aflags =0 ;unsigned blocks =1 ; +int delete_prevented =0 ;struct gfs2_diradd da = { .bh =NULL , .save_loc =1 , };if (!name->len || name->len >GFS2_FNAMESIZE )
@@ -705,6 +706,17 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,if (error) goto fail_free_inode; +/* +* Cancel any pending delete work for this glock. If there's pending +* delete work, we'd otherwise try to delete the dinode, but since we +* were assigned this address by alloc_dinode, the block is already +* free, so there's no need to attempt to change it from unlinked to +* free. We'd just get into trouble trying to do so. The biggest +* problem is having gfs2_delete_inode called while there pages +* still in existence due to a race between create and delete. +*/ +if (cancel_work_sync (&ip->i_gl->gl_delete)) + delete_prevented =1 ; ip->i_gl->gl_object = ip; error =gfs2_glock_nq_init (ip->i_gl,LM_ST_EXCLUSIVE ,GL_SKIP , ghs +1 );if (error)
@@ -762,6 +774,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, *opened |=FILE_CREATED ; error =finish_open (file, dentry, gfs2_open_common, opened); } +if (delete_prevented) { +gfs2_glock_put (ip->i_gl);/* fix the gl reference count */ +clear_bit (GLF_INODE_DELETING , &ip->i_gl->gl_flags); + }gfs2_glock_dq_uninit (ghs);gfs2_glock_dq_uninit (ghs +1 );return error;
@@ -772,8 +788,11 @@ fail_gunlock3: fail_gunlock2:gfs2_glock_dq_uninit (ghs +1 ); fail_free_inode: -if (ip->i_gl) +if (ip->i_gl) { +if (delete_prevented) +clear_bit (GLF_INODE_DELETING , &ip->i_gl->gl_flags);gfs2_glock_put (ip->i_gl); + }gfs2_rs_delete (ip,NULL ); fail_free_acls:if (default_acl)
fs/gfs2/rgrp.c +6 -0
@@ -1807,6 +1807,12 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skipif (error)continue ; +/* Make sure we're not queuing a delete if someone else has */ +if (test_and_set_bit (GLF_INODE_DELETING , &gl->gl_flags)) { +gfs2_glock_put (gl); +continue ; + } +/* If the inode is already in cache, we can ignore it here * because the existing inode disposal code will deal with * it when all refs have gone away. Accessing gl_object like
Signature · 1 line
2.4.3
PATCH v3 6/14 GFS2: Prevent gl_delete work for re-used inodes
Hi,
Quoted 8 lines · Bob Peterson
This patch adds a new glock flag
GLF_INODE_DELETINGwhich signifies when a glock is being used to change an inode from unlinked to deleted. The flag is used in a few places:
1. If an iopen callback is received, it checks the flag. If the bit is set, someone else has already started deleting the inode. In that case, thedelete_funcmay already be running, so we don't want to queue it to run another time. Doing so only gets us into trouble.
That is ok provided you are sure that we cannot miss any deletions in that case (i.e. the running deletion must succeed). There should be no problem with queuing up multiple requests for this, since subsequent ones will see that it has already gone.
Quoted 11 lines
2. When a dinode is in the process of being created, we've been assigned that block by the allocator, so it must have been free. At that point, we check if there is pending delete work pending, and if so, we cancel it to prevent the block from being deleted while we're creating it. This is necessary because there could be pending delete work that was queued up a while ago, but the delete work might have been done on another node, which is how the block became freed. However, we keep the
GLF_INODE_DELETINGset to prevent new delete work from being queued. After we're done creating, we clear the bit, otherwise the file may not be deleted ever again, even in legitimate cases in the future.
When inodes are created, then they will only be created from free blocks, so that they cannot be confused by any pending deletes which must run only on blocks marked as unlinked inodes. There will be a log flush between the unlink and the reuse at a minimum, so that should prevent this from happening. Is the issue that the log flush is not providing enough of a barrier I wonder?
Quoted 2 lines
3. In function
try_rgrp_unlink, we also make sure the bit isn't already set before we try to reclaim an unlinked block.
Ok, that makes sense to me,
Steve.
Quoted 120 lines
Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/glock.c | 2 ++ fs/gfs2/glops.c | 3 ++- fs/gfs2/incore.h | 1 + fs/gfs2/inode.c | 21 ++++++++++++++++++++- fs/gfs2/rgrp.c | 6 ++++++ 5 files changed, 31 insertions(+), 2 deletions(-)fs/gfs2/glock.c +2
-0@@ -1628,6 +1628,8 @@ static const char *gflags2str(char *buf, const struct gfs2_glock *gl) *p++ ='o' ;if (test_bit (GLF_BLOCKING , gflags)) *p++ ='b' ; +if (test_bit (GLF_INODE_DELETING , gflags)) + *p++ ='x' ; *p =0 ;return buf; }fs/gfs2/glops.c +2
-1@@ -553,7 +553,8 @@ static void iopen_go_callback(struct gfs2_glock *gl, bool remote)return ;if (gl->gl_demote_state ==LM_ST_UNLOCKED && - gl->gl_state ==LM_ST_SHARED && ip) { + gl->gl_state ==LM_ST_SHARED && ip && + !test_and_set_bit (GLF_INODE_DELETING , &gl->gl_flags)) { gl->gl_lockref.count++;if (queue_work (gfs2_delete_workqueue, &gl->gl_delete) ==0 ) gl->gl_lockref.count--;fs/gfs2/incore.h +1
-0@@ -326,6 +326,7 @@ enum {GLF_LRU =13 ,GLF_OBJECT =14 ,/* Used only for tracing */ GLF_BLOCKING =15 , + GLF_INODE_DELETING = 16, /* Was unlinked, being deleted */ }; struct gfs2_glock {fs/gfs2/inode.c +20
-1@@ -597,6 +597,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,int error, free_vfs_inode =0 ;u32 aflags =0 ;unsigned blocks =1 ; +int delete_prevented =0 ;struct gfs2_diradd da = { .bh =NULL , .save_loc =1 , };if (!name->len || name->len >GFS2_FNAMESIZE )@@ -705,6 +706,17 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,if (error) goto fail_free_inode; +/* +* Cancel any pending delete work for this glock. If there's pending +* delete work, we'd otherwise try to delete the dinode, but since we +* were assigned this address by alloc_dinode, the block is already +* free, so there's no need to attempt to change it from unlinked to +* free. We'd just get into trouble trying to do so. The biggest +* problem is having gfs2_delete_inode called while there pages +* still in existence due to a race between create and delete. +*/ +if (cancel_work_sync (&ip->i_gl->gl_delete)) + delete_prevented =1 ; ip->i_gl->gl_object = ip; error =gfs2_glock_nq_init (ip->i_gl,LM_ST_EXCLUSIVE ,GL_SKIP , ghs +1 );if (error)@@ -762,6 +774,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, *opened |=FILE_CREATED ; error =finish_open (file, dentry, gfs2_open_common, opened); } +if (delete_prevented) { +gfs2_glock_put (ip->i_gl);/* fix the gl reference count */ +clear_bit (GLF_INODE_DELETING , &ip->i_gl->gl_flags); + }gfs2_glock_dq_uninit (ghs);gfs2_glock_dq_uninit (ghs +1 );return error;@@ -772,8 +788,11 @@ fail_gunlock3: fail_gunlock2:gfs2_glock_dq_uninit (ghs +1 ); fail_free_inode: -if (ip->i_gl) +if (ip->i_gl) { +if (delete_prevented) +clear_bit (GLF_INODE_DELETING , &ip->i_gl->gl_flags);gfs2_glock_put (ip->i_gl); + }gfs2_rs_delete (ip,NULL ); fail_free_acls:if (default_acl)fs/gfs2/rgrp.c +6
-0@@ -1807,6 +1807,12 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skipif (error)continue ; +/* Make sure we're not queuing a delete if someone else has */ +if (test_and_set_bit (GLF_INODE_DELETING , &gl->gl_flags)) { +gfs2_glock_put (gl); +continue ; + } +/* If the inode is already in cache, we can ignore it here * because the existing inode disposal code will deal with * it when all refs have gone away. Accessing gl_object like
PATCH v3 7/14 GFS2: Truncate address space mapping when deleting an inode
In reply to Bob PetersonIn function gfs2_delete_inode() we write and flush the mapping for a glock, among other things. We truncate the mapping for the inode, but we never truncate the mapping for the glock. This patch makes it also truncate the metamapping. This avoid cases where the glock is reused by another process who is trying to recreate an inode in its place using the same block.
--- fs/gfs2/super.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
fs/gfs2/super.c +3 -1
@@ -1511,6 +1511,7 @@ static void gfs2_evict_inode(struct inode *inode)struct gfs2_sbd *sdp = sb->s_fs_info;struct gfs2_inode *ip =GFS2_I (inode);struct gfs2_holder gh; +struct address_space *metamapping;int error;if (test_bit (GIF_FREE_VFS_INODE , &ip->i_flags)) {
@@ -1575,8 +1576,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)) { -struct address_space *metamapping =gfs2_glock2aspace (ip->i_gl);filemap_fdatawrite (metamapping);filemap_fdatawait (metamapping); }
@@ -1589,6 +1590,7 @@ out_truncate: goto out_unlock;/* Needs to be done before glock release & also in a transaction */ truncate_inode_pages (&inode->i_data,0 ); +truncate_inode_pages (metamapping,0 );gfs2_trans_end (sdp); out_unlock:
Signature · 1 line
2.4.3
PATCH v3 7/14 GFS2: Truncate address space mapping when deleting an inode
Hi,
Steve.
Quoted 42 lines · Bob Peterson
In function
gfs2_delete_inode()we write and flush the mapping for a glock, among other things. We truncate the mapping for the inode, but we never truncate the mapping for the glock. This patch makes it also truncate the metamapping. This avoid cases where the glock is reused by another process who is trying to recreate an inode in its place using the same block.Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/super.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)fs/gfs2/super.c +3
-1@@ -1511,6 +1511,7 @@ static void gfs2_evict_inode(struct inode *inode)struct gfs2_sbd *sdp = sb->s_fs_info;struct gfs2_inode *ip =GFS2_I (inode);struct gfs2_holder gh; +struct address_space *metamapping;int error;if (test_bit (GIF_FREE_VFS_INODE , &ip->i_flags)) {@@ -1575,8 +1576,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)) { -struct address_space *metamapping =gfs2_glock2aspace (ip->i_gl);filemap_fdatawrite (metamapping);filemap_fdatawait (metamapping); }@@ -1589,6 +1590,7 @@ out_truncate: goto out_unlock;/* Needs to be done before glock release & also in a transaction */ truncate_inode_pages (&inode->i_data,0 ); +truncate_inode_pages (metamapping,0 );gfs2_trans_end (sdp); out_unlock:
PATCH v3 8/14 GFS2: Don't filter out I_FREEING inodes anymore
In reply to Bob PetersonThis patch basically reverts a very old patch from 2008, 7a9f53b3c1875bef22ad4588e818bc046ef183da, with the title "Alternate gfs2_iget to avoid looking up inodes being freed". The original patch was designed to avoid a deadlock caused by lock ordering with try_rgrp_unlink. The patch forced the function to not find inodes that were being removed by VFS. The problem is, that made it impossible for nodes to delete their own unlinked dinodes after a certain point in time, because the inode needed was not found by this filtering process. There is no longer a need for the patch, since function try_rgrp_unlink no longer locks the inode: All it does is queue the glock onto the delete work_queue, so there should be no more deadlock.
--- fs/gfs2/export.c | 2 +- fs/gfs2/glock.c | 2 +- fs/gfs2/inode.c | 59 ++++---------------------------------------------------- fs/gfs2/inode.h | 2 +- 4 files changed, 7 insertions(+), 58 deletions(-)
fs/gfs2/export.c +1 -1
@@ -137,7 +137,7 @@ static struct dentry *gfs2_get_dentry(struct super_block *sb,struct gfs2_sbd *sdp = sb->s_fs_info;struct inode *inode; - inode =gfs2_ilookup (sb, inum->no_addr,0 ); + inode =gfs2_ilookup (sb, inum->no_addr);if (inode) {if (GFS2_I (inode)->i_no_formal_ino != inum->no_formal_ino) {iput (inode);
fs/gfs2/glock.c +1 -1
@@ -576,7 +576,7 @@ static void delete_work_func(struct work_struct *work)/* Note: Unsafe to dereference ip as we don't hold right refs/locks */ if (ip) - inode =gfs2_ilookup (sdp->sd_vfs, no_addr,1 ); + inode =gfs2_ilookup (sdp->sd_vfs, no_addr);else inode =gfs2_lookup_by_inum (sdp, no_addr,NULL ,GFS2_BLKST_UNLINKED );if (inode && !IS_ERR (inode)) {
fs/gfs2/inode.c +4 -55
73 lines not on this page · raw message
fs/gfs2/inode.h +1 -1
@@ -99,7 +99,7 @@ extern struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type,extern struct inode *gfs2_lookup_by_inum (struct gfs2_sbd *sdp,u64 no_addr,u64 *no_formal_ino,unsigned int blktype); -extern struct inode *gfs2_ilookup (struct super_block *sb,u64 no_addr,int nonblock); +extern struct inode *gfs2_ilookup (struct super_block *sb,u64 no_addr);extern int gfs2_inode_refresh (struct gfs2_inode *ip);
Signature · 1 line
2.4.3
PATCH v3 8/14 GFS2: Don't filter out I_FREEING inodes anymore
Hi,
Quoted 4 lines · Bob Peterson
This patch basically reverts a very old patch from 2008,
7a9f53b3c1875bef22ad4588e818bc046ef183da, with the title "Alternategfs2_igetto avoid looking up inodes being freed". The original patch was designed to avoid a deadlock caused by lock
Quoted 8 lines
ordering with
try_rgrp_unlink. The patch forced the function to not find inodes that were being removed by VFS. The problem is, that made it impossible for nodes to delete their own unlinked dinodes after a certain point in time, because the inode needed was not found by this filtering process. There is no longer a need for the patch, since functiontry_rgrp_unlinkno longer locks the inode: All it does is queue the glock onto the deletework_queue, so there should be no more deadlock.
I'm not sure I understand. Why would we need to look up and inode in one of the
I_FREEING|I_CLEAR|I_WILL_FREE states? If the inode is in one of those states then it is already being removed, so we should be able to leave it to its own devices and it will be gone, in the normal case. This will also likely not be deadlock free for older kernels with the previous workqueue implementation, but will probably be ok for upstream,
Steve.
Quoted 126 lines
Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/export.c | 2 +- fs/gfs2/glock.c | 2 +- fs/gfs2/inode.c | 59 ++++---------------------------------------------------- fs/gfs2/inode.h | 2 +- 4 files changed, 7 insertions(+), 58 deletions(-)fs/gfs2/export.c +1
-1@@ -137,7 +137,7 @@ static struct dentry *gfs2_get_dentry(struct super_block *sb,struct gfs2_sbd *sdp = sb->s_fs_info;struct inode *inode; - inode =gfs2_ilookup (sb, inum->no_addr,0 ); + inode =gfs2_ilookup (sb, inum->no_addr);if (inode) {if (GFS2_I (inode)->i_no_formal_ino != inum->no_formal_ino) {iput (inode);fs/gfs2/glock.c +1
-1@@ -576,7 +576,7 @@ static void delete_work_func(struct work_struct *work)/* Note: Unsafe to dereference ip as we don't hold right refs/locks */ if (ip) - inode =gfs2_ilookup (sdp->sd_vfs, no_addr,1 ); + inode =gfs2_ilookup (sdp->sd_vfs, no_addr);else inode =gfs2_lookup_by_inum (sdp, no_addr,NULL ,GFS2_BLKST_UNLINKED );if (inode && !IS_ERR (inode)) {fs/gfs2/inode.c +4
-55@@ -37,61 +37,9 @@#include "super.h" #include "glops.h" -struct gfs2_skip_data { -u64 no_addr; -int skipped; -int non_block; -}; - -static int iget_test (struct inode *inode,void *opaque) -{ -struct gfs2_inode *ip =GFS2_I (inode); -struct gfs2_skip_data *data = opaque; - -if (ip->i_no_addr == data->no_addr) { -if (data->non_block && - inode->i_state & (I_FREEING |I_CLEAR |I_WILL_FREE )) { - data->skipped =1 ; -return 0 ; - } -return 1 ; - } -return 0 ; -} - -static int iget_set (struct inode *inode,void *opaque) +struct inode *gfs2_ilookup (struct super_block *sb,u64 no_addr) { -struct gfs2_inode *ip =GFS2_I (inode); -struct gfs2_skip_data *data = opaque; - -if (data->skipped) -return -ENOENT ; - inode->i_ino = (unsigned long )(data->no_addr); - ip->i_no_addr = data->no_addr; -return 0 ; -} - -struct inode *gfs2_ilookup (struct super_block *sb,u64 no_addr,int non_block) -{ -unsigned long hash = (unsigned long )no_addr; -struct gfs2_skip_data data; - - data.no_addr = no_addr; - data.skipped =0 ; - data.non_block = non_block; -return ilookup5 (sb, hash, iget_test, &data); -} - -static struct inode *gfs2_iget (struct super_block *sb,u64 no_addr, -int non_block) -{ -struct gfs2_skip_data data; -unsigned long hash = (unsigned long )no_addr; - - data.no_addr = no_addr; - data.skipped =0 ; - data.non_block = non_block; -return iget5_locked (sb, hash, iget_test, iget_set, &data); +return ilookup (sb, (unsigned long )no_addr); }/** @@ -145,8 +93,9 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,struct gfs2_glock *io_gl =NULL ;int error; - inode =gfs2_iget (sb, no_addr, non_block); + inode =iget_locked (sb, (unsigned long )no_addr); ip =GFS2_I (inode); + ip->i_no_addr = no_addr;if (!inode)return ERR_PTR (-ENOMEM );fs/gfs2/inode.h +1
-1@@ -99,7 +99,7 @@ extern struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type,extern struct inode *gfs2_lookup_by_inum (struct gfs2_sbd *sdp,u64 no_addr,u64 *no_formal_ino,unsigned int blktype); -extern struct inode *gfs2_ilookup (struct super_block *sb,u64 no_addr,int nonblock); +extern struct inode *gfs2_ilookup (struct super_block *sb,u64 no_addr);extern int gfs2_inode_refresh (struct gfs2_inode *ip);
PATCH v3 8/14 GFS2: Don't filter out I_FREEING inodes anymore
----- Original Message -----
Quoted 23 lines
Hi,
Quoted 4 lines · Bob Peterson
This patch basically reverts a very old patch from 2008,
7a9f53b3c1875bef22ad4588e818bc046ef183da, with the title "Alternategfs2_igetto avoid looking up inodes being freed". The original patch was designed to avoid a deadlock caused by lockQuoted 8 lines
ordering with
try_rgrp_unlink. The patch forced the function to not find inodes that were being removed by VFS. The problem is, that made it impossible for nodes to delete their own unlinked dinodes after a certain point in time, because the inode needed was not found by this filtering process. There is no longer a need for the patch, since functiontry_rgrp_unlinkno longer locks the inode: All it does is queue the glock onto the deletework_queue, so there should be no more deadlock.I'm not sure I understand. Why would we need to look up and inode in one of the
I_FREEING|I_CLEAR|I_WILL_FREE states? If the inode is in one of those states then it is already being removed, so we should be able to leave it to its own devices and it will be gone, in the normal case. This will also likely not be deadlock free for older kernels with the previous workqueue implementation, but will probably be ok for upstream,
Quoted 1 line
Steve.
Hi,
Sorry I haven't responded earlier. I've been reworking a lot of these patches and I didn't want to respond until I got further along.
This is the majority of the problem with not deleting/freeing unlinked inodes.
The problem is that today, we're filtering out inodes that are I_FREEING, which means I_FREEING inodes are treated as "not found" in the code that deletes the unlinked inodes, which leaves the block out there, unlinked.
What we should be doing is using the standard Linux mechanism, which means if the inode is I_FREEING, it should wait for it to be freed by vfs, IOW, block on __wait_on_freeing_inode(). When vfs (and gfs2) are done freeing the inode, a new inode will be created, and that one used for transitioning the block from unlinked to free. That closes the timing window to ensure there aren't blocks left in the UNLINKED state.
The problem is that I needed to jump through some hoops to ensure that the problem fixed by reverted patch 7a9f53b3c1875bef22ad4588e818bc046ef183da wasn't reintroduced. I had to ensure it was fixed via another mechanism.
Regards,
Bob Peterson
Red Hat File Systems
PATCH v3 9/14 GFS2: generalize gfs2_check_blk_type
In reply to Bob PetersonThis patch reorganizes function gfs2_check_blk_type so that a more generalized function, gfs2_get_block_type, may be used in future enhancements related to the freeing of unlinked dinodes.
--- fs/gfs2/rgrp.c | 46 ++++++++++++++++++++++++++++++++-------------- fs/gfs2/rgrp.h | 1 + 2 files changed, 33 insertions(+), 14 deletions(-)
fs/gfs2/rgrp.c +32 -14
@@ -2125,14 +2125,14 @@ void gfs2_inplace_release(struct gfs2_inode *ip) }/** -* gfs2_get_block_type - Check a block in a RG is of given type +* __get_block_type - Check a block in a RG is of given type * @rgd: the resource group holding the block * @block: the block number * * Returns: The block type (GFS2_BLKST_*) */ -static unsigned char gfs2_get_block_type (struct gfs2_rgrpd *rgd,u64 block) +static unsigned char __get_block_type (struct gfs2_rgrpd *rgd,u64 block) {struct gfs2_rbm rbm = { .rgd = rgd, };int ret;
@@ -2518,7 +2518,7 @@ void gfs2_free_di(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip) }/** -* gfs2_check_blk_type - Check the type of a block +* gfs2_get_blk_type - Get the type of a block * @sdp: The superblock * @no_addr: The block number to check * @type: The block type we are looking for
@@ -2528,26 +2528,44 @@ void gfs2_free_di(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip)* or -ve errno if something went wrong while checking */ -int gfs2_check_blk_type (struct gfs2_sbd *sdp,u64 no_addr,unsigned int type) +int gfs2_get_blk_type (struct gfs2_sbd *sdp,u64 no_addr) {struct gfs2_rgrpd *rgd;struct gfs2_holder rgd_gh; -int error = -EINVAL ; +int ret; rgd =gfs2_blk2rgrpd (sdp, no_addr,1 );if (!rgd) - goto fail; +return -EINVAL ; - error =gfs2_glock_nq_init (rgd->rd_gl,LM_ST_SHARED ,0 , &rgd_gh); -if (error) - goto fail; + ret =gfs2_glock_nq_init (rgd->rd_gl,LM_ST_SHARED ,0 , &rgd_gh); +if (ret ==0 ) { + ret =__get_block_type (rgd, no_addr); +gfs2_glock_dq_uninit (&rgd_gh); + } +return ret; +} -if (gfs2_get_block_type (rgd, no_addr) != type) - error = -ESTALE ; +/** +* gfs2_check_blk_type - Check the type of a block +* @sdp: The superblock +* @no_addr: The block number to check +* @type: The block type we are looking for +* +* Returns: 0 if the block type matches the expected type +* -ESTALE if it doesn't match +* or -ve errno if something went wrong while checking +*/ -gfs2_glock_dq_uninit (&rgd_gh); -fail: -return error; +int gfs2_check_blk_type (struct gfs2_sbd *sdp,u64 no_addr,unsigned int type) +{ +int ty =gfs2_get_blk_type (sdp, no_addr); + +if (ty <0 ) +return ty; +if (ty != type) +return -ESTALE ; +return 0 ; }/**
fs/gfs2/rgrp.h +1 -0
@@ -56,6 +56,7 @@ extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, intextern void gfs2_free_meta (struct gfs2_inode *ip,u64 bstart,u32 blen);extern void gfs2_free_di (struct gfs2_rgrpd *rgd,struct gfs2_inode *ip);extern void gfs2_unlink_di (struct inode *inode); +extern int gfs2_get_blk_type (struct gfs2_sbd *sdp,u64 no_addr);extern int gfs2_check_blk_type (struct gfs2_sbd *sdp,u64 no_addr,unsigned int type);
Signature · 1 line
2.4.3
PATCH v3 10/14 GFS2: Change from tr_touched to tr_bufs
In reply to Bob PetersonThis patch changes the GFS2 transaction processing so that it keeps track of how many buffers are associated with a transaction rather than whether or not it's been touched. That way, if a buffer is added, then revoked, then removed entirely, it can avoid some work.
--- fs/gfs2/incore.h | 5 ++++- fs/gfs2/meta_io.c | 6 +++--- fs/gfs2/trans.c | 13 +++++++------ 3 files changed, 14 insertions(+), 10 deletions(-)
fs/gfs2/incore.h +4 -1
@@ -473,10 +473,13 @@ struct gfs2_trans {unsigned int tr_blocks;unsigned int tr_revokes;unsigned int tr_reserved; - unsigned int tr_touched:1; unsigned int tr_attached:1; unsigned int tr_alloced:1; +struct gfs2_holder tr_t_gh; + +int tr_bufs; +unsigned int tr_num_buf_new;unsigned int tr_num_databuf_new;unsigned int tr_num_buf_rm;
fs/gfs2/meta_io.c +3 -3
@@ -224,7 +224,7 @@ int gfs2_meta_read(struct gfs2_glock *gl, u64 blkno, int flags,wait_on_buffer (bh);if (unlikely (!buffer_uptodate (bh))) {struct gfs2_trans *tr = current->journal_info; -if (tr && tr->tr_touched) +if (tr && tr->tr_bufs)gfs2_io_error_bh (sdp, bh);brelse (bh); *bhp =NULL ;
@@ -251,7 +251,7 @@ int gfs2_meta_wait(struct gfs2_sbd *sdp, struct buffer_head *bh)if (!buffer_uptodate (bh)) {struct gfs2_trans *tr = current->journal_info; -if (tr && tr->tr_touched) +if (tr && tr->tr_bufs)gfs2_io_error_bh (sdp, bh);return -EIO ; }
@@ -276,7 +276,7 @@ void gfs2_remove_from_journal(struct buffer_head *bh, struct gfs2_trans *tr, int tr->tr_num_buf_rm++; else tr->tr_num_databuf_rm++; - tr->tr_touched =1 ; + tr->tr_bufs--; was_pinned =1 ;brelse (bh); }
fs/gfs2/trans.c +7 -6
@@ -77,8 +77,8 @@ fail:static void gfs2_print_trans (const struct gfs2_trans *tr) {pr_warn ("Transaction created at: %pSR\n" , (void *)tr->tr_ip); -pr_warn ("blocks=%u revokes=%u reserved=%u touched=%u\n" , - tr->tr_blocks, tr->tr_revokes, tr->tr_reserved, tr->tr_touched); +pr_warn ("blocks=%u revokes=%u reserved=%u bufs=%d\n" , + tr->tr_blocks, tr->tr_revokes, tr->tr_reserved, tr->tr_bufs); pr_warn("Buf %u/%u Databuf %u/%u Revoke %u/%u\n", tr->tr_num_buf_new, tr->tr_num_buf_rm, tr->tr_num_databuf_new, tr->tr_num_databuf_rm,
@@ -94,7 +94,7 @@ void gfs2_trans_end(struct gfs2_sbd *sdp)BUG_ON (!tr); current->journal_info =NULL ; -if (!tr->tr_touched) { +if (tr->tr_bufs ==0 ) {gfs2_log_release (sdp, tr->tr_reserved);if (alloced) {kfree (tr);
@@ -182,7 +182,7 @@ void gfs2_trans_add_data(struct gfs2_glock *gl, struct buffer_head *bh)gfs2_log_lock (sdp); }gfs2_assert (sdp, bd->bd_gl == gl); - tr->tr_touched =1 ; + tr->tr_bufs++;if (list_empty (&bd->bd_list)) {set_bit (GLF_LFLUSH , &bd->bd_gl->gl_flags);set_bit (GLF_DIRTY , &bd->bd_gl->gl_flags);
@@ -201,7 +201,7 @@ static void meta_lo_add(struct gfs2_sbd *sdp, struct gfs2_bufdata *bd)enum gfs2_freeze_state state =atomic_read (&sdp->sd_freeze_state); tr = current->journal_info; - tr->tr_touched =1 ; + tr->tr_bufs++;if (!list_empty (&bd->bd_list))return ;set_bit (GLF_LFLUSH , &bd->bd_gl->gl_flags);
@@ -256,7 +256,7 @@ void gfs2_trans_add_revoke(struct gfs2_sbd *sdp, struct gfs2_bufdata *bd)BUG_ON (!list_empty (&bd->bd_list));gfs2_add_revoke (sdp, bd); - tr->tr_touched =1 ; + tr->tr_bufs++; tr->tr_num_revoke++; }
@@ -272,6 +272,7 @@ void gfs2_trans_add_unrevoke(struct gfs2_sbd *sdp, u64 blkno, unsigned int len)list_del_init (&bd->bd_list);gfs2_assert_withdraw (sdp, sdp->sd_log_num_revoke); sdp->sd_log_num_revoke--; + tr->tr_bufs--;kmem_cache_free (gfs2_bufdata_cachep, bd); tr->tr_num_revoke_rm++;if (--n ==0 )
Signature · 1 line
2.4.3
PATCH v3 10/14 GFS2: Change from tr_touched to tr_bufs
Hi,
Quoted 4 lines · Bob Peterson
This patch changes the GFS2 transaction processing so that it keeps track of how many buffers are associated with a transaction rather than whether or not it's been touched. That way, if a buffer is added, then revoked, then removed entirely, it can avoid some work.
How does this relate to the issue of fixing the unlink/delete code path? It looks like this is something that should be a separate patch and not part of this series,
Steve.
Quoted 116 lines
Signed-off-by Bob Peterson <[email protected]>--- fs/gfs2/incore.h | 5 ++++- fs/gfs2/meta_io.c | 6 +++--- fs/gfs2/trans.c | 13 +++++++------ 3 files changed, 14 insertions(+), 10 deletions(-)fs/gfs2/incore.h +4
-1@@ -473,10 +473,13 @@ struct gfs2_trans {unsigned int tr_blocks;unsigned int tr_revokes;unsigned int tr_reserved; - unsigned int tr_touched:1; unsigned int tr_attached:1; unsigned int tr_alloced:1; +struct gfs2_holder tr_t_gh; + +int tr_bufs; +unsigned int tr_num_buf_new;unsigned int tr_num_databuf_new;unsigned int tr_num_buf_rm;fs/gfs2/meta_io.c +3
-3@@ -224,7 +224,7 @@ int gfs2_meta_read(struct gfs2_glock *gl, u64 blkno, int flags,wait_on_buffer (bh);if (unlikely (!buffer_uptodate (bh))) {struct gfs2_trans *tr = current->journal_info; -if (tr && tr->tr_touched) +if (tr && tr->tr_bufs)gfs2_io_error_bh (sdp, bh);brelse (bh); *bhp =NULL ;@@ -251,7 +251,7 @@ int gfs2_meta_wait(struct gfs2_sbd *sdp, struct buffer_head *bh)if (!buffer_uptodate (bh)) {struct gfs2_trans *tr = current->journal_info; -if (tr && tr->tr_touched) +if (tr && tr->tr_bufs)gfs2_io_error_bh (sdp, bh);return -EIO ; }@@ -276,7 +276,7 @@ void gfs2_remove_from_journal(struct buffer_head *bh, struct gfs2_trans *tr, int tr->tr_num_buf_rm++; else tr->tr_num_databuf_rm++; - tr->tr_touched =1 ; + tr->tr_bufs--; was_pinned =1 ;brelse (bh); }fs/gfs2/trans.c +7
-6@@ -77,8 +77,8 @@ fail:static void gfs2_print_trans (const struct gfs2_trans *tr) {pr_warn ("Transaction created at: %pSR\n" , (void *)tr->tr_ip); -pr_warn ("blocks=%u revokes=%u reserved=%u touched=%u\n" , - tr->tr_blocks, tr->tr_revokes, tr->tr_reserved, tr->tr_touched); +pr_warn ("blocks=%u revokes=%u reserved=%u bufs=%d\n" , + tr->tr_blocks, tr->tr_revokes, tr->tr_reserved, tr->tr_bufs); pr_warn("Buf %u/%u Databuf %u/%u Revoke %u/%u\n", tr->tr_num_buf_new, tr->tr_num_buf_rm, tr->tr_num_databuf_new, tr->tr_num_databuf_rm,@@ -94,7 +94,7 @@ void gfs2_trans_end(struct gfs2_sbd *sdp)BUG_ON (!tr); current->journal_info =NULL ; -if (!tr->tr_touched) { +if (tr->tr_bufs ==0 ) {gfs2_log_release (sdp, tr->tr_reserved);if (alloced) {kfree (tr);@@ -182,7 +182,7 @@ void gfs2_trans_add_data(struct gfs2_glock *gl, struct buffer_head *bh)gfs2_log_lock (sdp); }gfs2_assert (sdp, bd->bd_gl == gl); - tr->tr_touched =1 ; + tr->tr_bufs++;if (list_empty (&bd->bd_list)) {set_bit (GLF_LFLUSH , &bd->bd_gl->gl_flags);set_bit (GLF_DIRTY , &bd->bd_gl->gl_flags);@@ -201,7 +201,7 @@ static void meta_lo_add(struct gfs2_sbd *sdp, struct gfs2_bufdata *bd)enum gfs2_freeze_state state =atomic_read (&sdp->sd_freeze_state); tr = current->journal_info; - tr->tr_touched =1 ; + tr->tr_bufs++;if (!list_empty (&bd->bd_list))return ;set_bit (GLF_LFLUSH , &bd->bd_gl->gl_flags);@@ -256,7 +256,7 @@ void gfs2_trans_add_revoke(struct gfs2_sbd *sdp, struct gfs2_bufdata *bd)BUG_ON (!list_empty (&bd->bd_list));gfs2_add_revoke (sdp, bd); - tr->tr_touched =1 ; + tr->tr_bufs++; tr->tr_num_revoke++; }@@ -272,6 +272,7 @@ void gfs2_trans_add_unrevoke(struct gfs2_sbd *sdp, u64 blkno, unsigned int len)list_del_init (&bd->bd_list);gfs2_assert_withdraw (sdp, sdp->sd_log_num_revoke); sdp->sd_log_num_revoke--; + tr->tr_bufs--;kmem_cache_free (gfs2_bufdata_cachep, bd); tr->tr_num_revoke_rm++;if (--n ==0 )
PATCH v3 11/14 GFS2: Add new function gfs2_inode_lookup_for_del
In reply to Bob PetersonThis patch adds a new specialized function to look up an inode for the purposes of deleting it. Before, we used to call function gfs2_lookup_by_inum, but the new function closes some timing windows involving the iopen and inode glocks coming and going, since they typically outlive their inodes.
--- fs/gfs2/glock.c | 9 ++-- fs/gfs2/inode.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ fs/gfs2/inode.h | 2 + 3 files changed, 140 insertions(+), 4 deletions(-)
fs/gfs2/glock.c +5 -4
@@ -569,16 +569,17 @@ static void delete_work_func(struct work_struct *work) struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_delete);struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;struct gfs2_inode *ip; -struct inode *inode; +struct inode *inode =NULL ;u64 no_addr = gl->gl_name.ln_number; ip = gl->gl_object;/* Note: Unsafe to dereference ip as we don't hold right refs/locks */ -if (ip) inode =gfs2_ilookup (sdp->sd_vfs, no_addr); -else - inode =gfs2_lookup_by_inum (sdp, no_addr,NULL ,GFS2_BLKST_UNLINKED ); + +if (inode ==NULL ||IS_ERR (inode)) + inode =gfs2_inode_lookup_for_del (sdp->sd_vfs, no_addr); +if (inode && !IS_ERR (inode)) {d_prune_aliases (inode);iput (inode);
fs/gfs2/inode.c +133 -0
139 lines not on this page · raw message
fs/gfs2/inode.h +2 -0
@@ -96,6 +96,8 @@ err:extern struct inode *gfs2_inode_lookup (struct super_block *sb,unsigned type,u64 no_addr,u64 no_formal_ino,int non_block); +extern struct inode *gfs2_inode_lookup_for_del (struct super_block *sb, +u64 no_addr);extern struct inode *gfs2_lookup_by_inum (struct gfs2_sbd *sdp,u64 no_addr,u64 *no_formal_ino,unsigned int blktype);
Signature · 1 line
2.4.3
PATCH v3 12/14 gfs2: Remove unused param non_block from gfs2_inode_lookup
In reply to Bob PetersonFunction gfs2_inode_lookup had parameter non_block which is no longer used. This patch removes it and fixes the comments and callers.
--- fs/gfs2/dir.c | 2 +- fs/gfs2/inode.c | 8 ++++---- fs/gfs2/inode.h | 3 +-- fs/gfs2/ops_fstype.c | 2 +- 4 files changed, 7 insertions(+), 8 deletions(-)
fs/gfs2/dir.c +1 -1
@@ -1563,7 +1563,7 @@ struct inode *gfs2_dir_search(struct inode *dir, const struct qstr *name,brelse (bh);if (fail_on_exist)return ERR_PTR (-EEXIST ); -return gfs2_inode_lookup (dir->i_sb, dtype, addr, formal_ino,0 ); +return gfs2_inode_lookup (dir->i_sb, dtype, addr, formal_ino); }return ERR_PTR (-ENOENT ); }
fs/gfs2/inode.c +4 -4
@@ -78,15 +78,15 @@ static void gfs2_set_iop(struct inode *inode)/** * gfs2_inode_lookup - Lookup an inode * @sb: The super block -* @no_addr: The inode number * @type: The type of the inode -* non_block: Can we block on inodes that are being freed? +* @no_addr: The inode number +* @no_formal_ino: The formal inode number (for NFS) * * Returns: A VFS inode, or an error */ struct inode *gfs2_inode_lookup (struct super_block *sb,unsigned int type, -u64 no_addr,u64 no_formal_ino,int non_block) +u64 no_addr,u64 no_formal_ino) {struct inode *inode;struct gfs2_inode *ip;
@@ -304,7 +304,7 @@ struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,if (error) goto fail; - inode =gfs2_inode_lookup (sb,DT_UNKNOWN , no_addr,0 ,1 ); + inode =gfs2_inode_lookup (sb,DT_UNKNOWN , no_addr,0 );if (IS_ERR (inode)) goto fail;
fs/gfs2/inode.h +1 -2
@@ -94,8 +94,7 @@ err: }extern struct inode *gfs2_inode_lookup (struct super_block *sb,unsigned type, -u64 no_addr,u64 no_formal_ino, -int non_block); +u64 no_addr,u64 no_formal_ino);extern struct inode *gfs2_inode_lookup_for_del (struct super_block *sb,u64 no_addr); extern struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
fs/gfs2/ops_fstype.c +1 -1
@@ -451,7 +451,7 @@ static int gfs2_lookup_root(struct super_block *sb, struct dentry **dptr,struct dentry *dentry;struct inode *inode; - inode =gfs2_inode_lookup (sb,DT_DIR , no_addr,0 ,0 ); + inode =gfs2_inode_lookup (sb,DT_DIR , no_addr,0 );if (IS_ERR (inode)) {fs_err (sdp,"can't read in %s inode: %ld\n" , name,PTR_ERR (inode));return PTR_ERR (inode);
Signature · 1 line
2.4.3
PATCH v3 13/14 GFS2: Hold onto iopen glock longer when dinode creation fails
In reply to Bob PetersonThis patch allows function gfs2_create_inode to keep hold of the iopen glock longer in cases where the creation of a new dinode fails. This prevents some nasty timing windows where a dinode is being created and reused.
--- fs/gfs2/inode.c | 19 ++++++++++++------- fs/gfs2/main.c | 2 ++ 2 files changed, 14 insertions(+), 7 deletions(-)
fs/gfs2/inode.c +12 -7
@@ -673,7 +673,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,struct posix_acl *default_acl, *acl;struct gfs2_holder ghs[2 ];struct inode *inode =NULL ; -struct gfs2_inode *dip =GFS2_I (dir), *ip; +struct gfs2_inode *dip =GFS2_I (dir), *ip =NULL ;struct gfs2_sbd *sdp =GFS2_SB (&dip->i_inode);struct gfs2_glock *io_gl;int error, free_vfs_inode =0 ;
@@ -839,16 +839,16 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, }if (error) - goto fail_gunlock3; + goto fail_gunlock2; error =security_inode_init_security (&ip->i_inode, &dip->i_inode, name, &gfs2_initxattrs,NULL );if (error) - goto fail_gunlock3; + goto fail_gunlock2; error =link_dinode (dip, name, ip, &da);if (error) - goto fail_gunlock3; + goto fail_gunlock2;mark_inode_dirty (inode);d_instantiate (dentry, inode);
@@ -864,9 +864,6 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,gfs2_glock_dq_uninit (ghs +1 );return error; -fail_gunlock3: -if (ip->i_iopen_gh.gh_gl)/* if holder is linked to the glock */ -gfs2_glock_put (ip->i_iopen_gh.gh_gl); fail_gunlock2:gfs2_glock_dq_uninit (ghs +1 ); fail_free_inode:
@@ -883,6 +880,14 @@ fail_free_acls:posix_acl_release (acl); fail_free_vfs_inode: free_vfs_inode =1 ; +/* We hold off until the very end to release the iopen glock. That +* keeps other processes from acquiring it in EX mode and deleting +* it while we're still using it. Since gfs2_delete_inode already +* handles the iopen vs. inode glocks in any order, the lock order +* does not matter. It must be done before iput, though, otherwise +* we might get a segfault trying to dereference it. */ +if (ip && ip->i_iopen_gh.gh_gl)/* if holder is linked to the glock */ +gfs2_glock_put (ip->i_iopen_gh.gh_gl); fail_gunlock:gfs2_dir_no_add (&da);gfs2_glock_dq_uninit (ghs);
fs/gfs2/main.c +2 -0
@@ -43,6 +43,8 @@ static void gfs2_init_inode_once(void *foo)INIT_LIST_HEAD (&ip->i_trunc_list); ip->i_res =NULL ; ip->i_hash_cache =NULL ; + ip->i_iopen_gh.gh_gl =NULL ; +INIT_LIST_HEAD (&ip->i_iopen_gh.gh_list); }static void gfs2_init_glock_once (void *foo)
Signature · 1 line
2.4.3
PATCH v3 14/14 GFS2: Rework gfs2_evict_inode to prevent collisions with openers
In reply to Bob PetersonThis patch mainly changes function gfs2_evict_inode so that it takes out glock references and sets a DELETING bit to prevent collisions from inode and glock creations from interfering. This is necessary to ensure a smooth transition from unlinked to deleted status for dinodes.
--- fs/gfs2/incore.h | 1 + fs/gfs2/meta_io.c | 6 ++++++ fs/gfs2/super.c | 35 ++++++++++++++++++++++++++++------- 3 files changed, 35 insertions(+), 7 deletions(-)
fs/gfs2/incore.h +1 -0
@@ -381,6 +381,7 @@ enum {GIF_SW_PAGED =3 ,GIF_ORDERED =4 ,GIF_FREE_VFS_INODE =5 , + GIF_AM_DELETING = 6, }; struct gfs2_inode {
fs/gfs2/meta_io.c +6 -0
@@ -128,6 +128,12 @@ struct buffer_head *gfs2_getbuf(struct gfs2_glock *gl, u64 blkno, int create) index = blkno >> shift;/* convert block to page */ bufnum = blkno - (index << shift);/* block buf index within page */ +if ((gl->gl_name.ln_type ==LM_TYPE_INODE ) && gl->gl_object) { +struct gfs2_inode *ip = gl->gl_object; +struct inode *inode = &ip->i_inode; +if (!test_bit (GIF_AM_DELETING , &ip->i_flags)) +BUG_ON (inode->i_state &I_FREEING ); + }if (create) {for (;;) { page =grab_cache_page (mapping, index);
fs/gfs2/super.c +28 -7
@@ -1511,6 +1511,7 @@ static void gfs2_evict_inode(struct inode *inode)struct gfs2_sbd *sdp = sb->s_fs_info;struct gfs2_inode *ip =GFS2_I (inode);struct gfs2_holder gh; +struct gfs2_glock *io_gl =NULL ;struct address_space *metamapping;int error;
@@ -1522,6 +1523,7 @@ static void gfs2_evict_inode(struct inode *inode)if (inode->i_nlink || (sb->s_flags &MS_RDONLY )) goto out; +BUG_ON (__lockref_is_dead (&ip->i_gl->gl_lockref));/* Must not read inode block until block type has been verified */ error =gfs2_glock_nq_init (ip->i_gl,LM_ST_EXCLUSIVE ,GL_SKIP , &gh);if (unlikely (error)) {
@@ -1531,10 +1533,13 @@ static void gfs2_evict_inode(struct inode *inode) goto out; } +set_bit (GIF_AM_DELETING , &ip->i_flags);if (!test_bit (GIF_ALLOC_FAILED , &ip->i_flags)) { error =gfs2_check_blk_type (sdp, ip->i_no_addr,GFS2_BLKST_UNLINKED ); -if (error) - goto out_truncate; +if (error) { +gfs2_glock_dq_uninit (&gh); + goto out; + } }if (test_bit (GIF_INVALID , &ip->i_flags)) {
@@ -1543,10 +1548,22 @@ static void gfs2_evict_inode(struct inode *inode) goto out_truncate; } +/* The iopen glock may have been released prior to this, but we need +it back. */ + error =gfs2_glock_get (sdp, ip->i_no_addr, &gfs2_iopen_glops,CREATE , + &io_gl); +if (error) + goto out_truncate; + ip->i_iopen_gh.gh_flags |=GL_NOCACHE ; -gfs2_glock_dq_wait (&ip->i_iopen_gh); -gfs2_holder_reinit (LM_ST_EXCLUSIVE ,LM_FLAG_TRY_1CB |GL_NOCACHE , &ip->i_iopen_gh); - error =gfs2_glock_nq (&ip->i_iopen_gh); +if (ip->i_iopen_gh.gh_gl && +test_bit (HIF_HOLDER , &ip->i_iopen_gh.gh_iflags)) { +gfs2_glock_dq_wait (&ip->i_iopen_gh); +gfs2_holder_uninit (&ip->i_iopen_gh); + } + + error =gfs2_glock_nq_init (io_gl,LM_ST_EXCLUSIVE ,LM_FLAG_TRY_1CB | +GL_NOCACHE , &ip->i_iopen_gh);if (error) goto out_truncate;
@@ -1598,11 +1615,15 @@ out_unlock:if (gfs2_rs_active (ip->i_res))gfs2_rs_deltree (ip->i_res); -if (test_bit (HIF_HOLDER , &ip->i_iopen_gh.gh_iflags)) { +if (ip->i_iopen_gh.gh_gl && +test_bit (HIF_HOLDER , &ip->i_iopen_gh.gh_iflags)) { ip->i_iopen_gh.gh_flags |=GL_NOCACHE ;gfs2_glock_dq_wait (&ip->i_iopen_gh); +gfs2_holder_uninit (&ip->i_iopen_gh); } -gfs2_holder_uninit (&ip->i_iopen_gh); +if (io_gl) +gfs2_glock_put (io_gl); +gfs2_glock_dq_uninit (&gh);if (error && error !=GLR_TRYFAILED && error != -EROFS )fs_warn (sdp,"gfs2_evict_inode: %d\n" , error);
Signature · 1 line
2.4.3