* [Cluster-devel] [GFS2 PATCH] GFS2: Increase i_writecount during gfs2_setattr_size [not found] <2055011372.29570526.1369749816847.JavaMail.root@redhat.com> @ 2013-05-28 14:04 ` Bob Peterson 2013-05-28 14:20 ` Steven Whitehouse 0 siblings, 1 reply; 4+ messages in thread From: Bob Peterson @ 2013-05-28 14:04 UTC (permalink / raw) To: cluster-devel.redhat.com Hi, This patch calls get_write_access in a few functions. This merely increases inode->i_writecount for the duration of the function. That will ensure that any file closes won't delete the inode's multi-block reservation while the function is running. Regards, Bob Peterson Red Hat File Systems Signed-off-by: Bob Peterson <rpeterso@redhat.com> --- diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c index 1dc9a13..3f8ad82 100644 --- a/fs/gfs2/bmap.c +++ b/fs/gfs2/bmap.c @@ -1286,17 +1286,26 @@ int gfs2_setattr_size(struct inode *inode, u64 newsize) if (ret) return ret; + ret = get_write_access(inode); + if (ret) + return ret; + inode_dio_wait(inode); ret = gfs2_rs_alloc(GFS2_I(inode)); if (ret) - return ret; + goto out; oldsize = inode->i_size; - if (newsize >= oldsize) - return do_grow(inode, newsize); + if (newsize >= oldsize) { + ret = do_grow(inode, newsize); + goto out; + } - return do_shrink(inode, oldsize, newsize); + ret = do_shrink(inode, oldsize, newsize); +out: + put_write_access(inode); + return ret; } int gfs2_truncatei_resume(struct gfs2_inode *ip) diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index acd1676..ad0dc38 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -402,16 +402,20 @@ static int gfs2_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) /* Update file times before taking page lock */ file_update_time(vma->vm_file); + ret = get_write_access(inode); + if (ret) + goto out; + ret = gfs2_rs_alloc(ip); if (ret) - return ret; + goto out_write_access; gfs2_size_hint(vma->vm_file, pos, PAGE_CACHE_SIZE); gfs2_holder_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh); ret = gfs2_glock_nq(&gh); if (ret) - goto out; + goto out_uninit; set_bit(GLF_DIRTY, &ip->i_gl->gl_flags); set_bit(GIF_SW_PAGED, &ip->i_flags); @@ -480,12 +484,15 @@ out_quota_unlock: gfs2_quota_unlock(ip); out_unlock: gfs2_glock_dq(&gh); -out: +out_uninit: gfs2_holder_uninit(&gh); if (ret == 0) { set_page_dirty(page); wait_for_stable_page(page); } +out_write_access: + put_write_access(inode); +out: sb_end_pagefault(inode->i_sb); return block_page_mkwrite_return(ret); } @@ -594,10 +601,10 @@ static int gfs2_release(struct inode *inode, struct file *file) kfree(file->private_data); file->private_data = NULL; - if ((file->f_mode & FMODE_WRITE) && - (atomic_read(&inode->i_writecount) == 1)) - gfs2_rs_delete(ip); + if (!(file->f_mode & FMODE_WRITE)) + return 0; + gfs2_rs_delete(ip); return 0; } diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c index 5232525..9809156 100644 --- a/fs/gfs2/rgrp.c +++ b/fs/gfs2/rgrp.c @@ -638,8 +638,10 @@ void gfs2_rs_deltree(struct gfs2_blkreserv *rs) */ void gfs2_rs_delete(struct gfs2_inode *ip) { + struct inode *inode = &ip->i_inode; + down_write(&ip->i_rw_mutex); - if (ip->i_res) { + if (ip->i_res && atomic_read(&inode->i_writecount) <= 1) { gfs2_rs_deltree(ip->i_res); BUG_ON(ip->i_res->rs_free); kmem_cache_free(gfs2_rsrv_cachep, ip->i_res); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Increase i_writecount during gfs2_setattr_size 2013-05-28 14:04 ` [Cluster-devel] [GFS2 PATCH] GFS2: Increase i_writecount during gfs2_setattr_size Bob Peterson @ 2013-05-28 14:20 ` Steven Whitehouse 2013-05-28 16:54 ` Bob Peterson 0 siblings, 1 reply; 4+ messages in thread From: Steven Whitehouse @ 2013-05-28 14:20 UTC (permalink / raw) To: cluster-devel.redhat.com Hi, On Tue, 2013-05-28 at 10:04 -0400, Bob Peterson wrote: > Hi, > > This patch calls get_write_access in a few functions. This > merely increases inode->i_writecount for the duration of the function. > That will ensure that any file closes won't delete the inode's > multi-block reservation while the function is running. > > Regards, > > Bob Peterson > Red Hat File Systems > > Signed-off-by: Bob Peterson <rpeterso@redhat.com> > --- > diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c > index 1dc9a13..3f8ad82 100644 > --- a/fs/gfs2/bmap.c > +++ b/fs/gfs2/bmap.c > @@ -1286,17 +1286,26 @@ int gfs2_setattr_size(struct inode *inode, u64 newsize) > if (ret) > return ret; > > + ret = get_write_access(inode); > + if (ret) > + return ret; > + > inode_dio_wait(inode); > > ret = gfs2_rs_alloc(GFS2_I(inode)); > if (ret) > - return ret; > + goto out; > > oldsize = inode->i_size; > - if (newsize >= oldsize) > - return do_grow(inode, newsize); > + if (newsize >= oldsize) { > + ret = do_grow(inode, newsize); > + goto out; > + } > > - return do_shrink(inode, oldsize, newsize); > + ret = do_shrink(inode, oldsize, newsize); > +out: > + put_write_access(inode); > + return ret; > } > > int gfs2_truncatei_resume(struct gfs2_inode *ip) > diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c > index acd1676..ad0dc38 100644 > --- a/fs/gfs2/file.c > +++ b/fs/gfs2/file.c > @@ -402,16 +402,20 @@ static int gfs2_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) > /* Update file times before taking page lock */ > file_update_time(vma->vm_file); > > + ret = get_write_access(inode); > + if (ret) > + goto out; > + > ret = gfs2_rs_alloc(ip); > if (ret) > - return ret; > + goto out_write_access; > > gfs2_size_hint(vma->vm_file, pos, PAGE_CACHE_SIZE); > > gfs2_holder_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, &gh); > ret = gfs2_glock_nq(&gh); > if (ret) > - goto out; > + goto out_uninit; > > set_bit(GLF_DIRTY, &ip->i_gl->gl_flags); > set_bit(GIF_SW_PAGED, &ip->i_flags); > @@ -480,12 +484,15 @@ out_quota_unlock: > gfs2_quota_unlock(ip); > out_unlock: > gfs2_glock_dq(&gh); > -out: > +out_uninit: > gfs2_holder_uninit(&gh); > if (ret == 0) { > set_page_dirty(page); > wait_for_stable_page(page); > } > +out_write_access: > + put_write_access(inode); > +out: > sb_end_pagefault(inode->i_sb); > return block_page_mkwrite_return(ret); > } > @@ -594,10 +601,10 @@ static int gfs2_release(struct inode *inode, struct file *file) > kfree(file->private_data); > file->private_data = NULL; > > - if ((file->f_mode & FMODE_WRITE) && > - (atomic_read(&inode->i_writecount) == 1)) > - gfs2_rs_delete(ip); > + if (!(file->f_mode & FMODE_WRITE)) > + return 0; > > + gfs2_rs_delete(ip); > return 0; > } > > diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c > index 5232525..9809156 100644 > --- a/fs/gfs2/rgrp.c > +++ b/fs/gfs2/rgrp.c > @@ -638,8 +638,10 @@ void gfs2_rs_deltree(struct gfs2_blkreserv *rs) > */ > void gfs2_rs_delete(struct gfs2_inode *ip) > { > + struct inode *inode = &ip->i_inode; > + > down_write(&ip->i_rw_mutex); > - if (ip->i_res) { > + if (ip->i_res && atomic_read(&inode->i_writecount) <= 1) { > gfs2_rs_deltree(ip->i_res); > BUG_ON(ip->i_res->rs_free); > kmem_cache_free(gfs2_rsrv_cachep, ip->i_res); > Are there any other callers of gfs2_rs_delete where it is no appropriate to have this new test? I assume that the issue is that this writecount test needs to be under the i_rw_mutex? Steve. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Increase i_writecount during gfs2_setattr_size 2013-05-28 14:20 ` Steven Whitehouse @ 2013-05-28 16:54 ` Bob Peterson 2013-05-29 10:44 ` Steven Whitehouse 0 siblings, 1 reply; 4+ messages in thread From: Bob Peterson @ 2013-05-28 16:54 UTC (permalink / raw) To: cluster-devel.redhat.com ----- Original Message ----- | > --- a/fs/gfs2/rgrp.c | > +++ b/fs/gfs2/rgrp.c | > @@ -638,8 +638,10 @@ void gfs2_rs_deltree(struct gfs2_blkreserv *rs) | > */ | > void gfs2_rs_delete(struct gfs2_inode *ip) | > { | > + struct inode *inode = &ip->i_inode; | > + | > down_write(&ip->i_rw_mutex); | > - if (ip->i_res) { | > + if (ip->i_res && atomic_read(&inode->i_writecount) <= 1) { | > gfs2_rs_deltree(ip->i_res); | > BUG_ON(ip->i_res->rs_free); | > kmem_cache_free(gfs2_rsrv_cachep, ip->i_res); | > | | Are there any other callers of gfs2_rs_delete where it is no appropriate | to have this new test? | | I assume that the issue is that this writecount test needs to be under | the i_rw_mutex? | | Steve. Hi, Nope. It's okay for reservations to go in and out of a rgrp reservations tree; it happens all the time. What we really need to protect is where it's freed from cache (kmem_cache_free) which only happens in function gfs2_rs_deltree, where it's now protected by this patch. And yes, it needs to be done under the i_rw_mutex. The bigger question is whether there are other places besides functions gfs2_setattr_size and gfs2_page_mkwrite that should be calling get_write_access to ensure this protection. These are the only two we've seen in actual practice, and I wanted the patch to be as minimal as possible. Regards, Bob Peterson Red Hat File Systems ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Increase i_writecount during gfs2_setattr_size 2013-05-28 16:54 ` Bob Peterson @ 2013-05-29 10:44 ` Steven Whitehouse 0 siblings, 0 replies; 4+ messages in thread From: Steven Whitehouse @ 2013-05-29 10:44 UTC (permalink / raw) To: cluster-devel.redhat.com Hi, On Tue, 2013-05-28 at 12:54 -0400, Bob Peterson wrote: > ----- Original Message ----- > | > --- a/fs/gfs2/rgrp.c > | > +++ b/fs/gfs2/rgrp.c > | > @@ -638,8 +638,10 @@ void gfs2_rs_deltree(struct gfs2_blkreserv *rs) > | > */ > | > void gfs2_rs_delete(struct gfs2_inode *ip) > | > { > | > + struct inode *inode = &ip->i_inode; > | > + > | > down_write(&ip->i_rw_mutex); > | > - if (ip->i_res) { > | > + if (ip->i_res && atomic_read(&inode->i_writecount) <= 1) { > | > gfs2_rs_deltree(ip->i_res); > | > BUG_ON(ip->i_res->rs_free); > | > kmem_cache_free(gfs2_rsrv_cachep, ip->i_res); > | > > | > | Are there any other callers of gfs2_rs_delete where it is no appropriate > | to have this new test? > | > | I assume that the issue is that this writecount test needs to be under > | the i_rw_mutex? > | > | Steve. > > Hi, > > Nope. It's okay for reservations to go in and out of a rgrp reservations tree; > it happens all the time. What we really need to protect is where it's > freed from cache (kmem_cache_free) which only happens in function > gfs2_rs_deltree, where it's now protected by this patch. And yes, it > needs to be done under the i_rw_mutex. > Ok, sounds good. I've applied the patch to the -nmw tree. > The bigger question is whether there are other places besides functions > gfs2_setattr_size and gfs2_page_mkwrite that should be calling > get_write_access to ensure this protection. These are the only two > we've seen in actual practice, and I wanted the patch to be as > minimal as possible. > > Regards, > > Bob Peterson > Red Hat File Systems Yes, looks good. If there are other places, they are likely to be those where we write to internal files via an interface other than the vfs. The quotactl interface comes to mind, and perhaps also statfs too. From userspace, if we have an open file descriptor, then there should generally not be a problem, Steve. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-29 10:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <2055011372.29570526.1369749816847.JavaMail.root@redhat.com>
2013-05-28 14:04 ` [Cluster-devel] [GFS2 PATCH] GFS2: Increase i_writecount during gfs2_setattr_size Bob Peterson
2013-05-28 14:20 ` Steven Whitehouse
2013-05-28 16:54 ` Bob Peterson
2013-05-29 10:44 ` Steven Whitehouse
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).