* [Cluster-devel] [PATCH] [GFS2] bz253089 Make gfs2_writepage use trylock on the log_flush lock
@ 2007-09-13 16:30 Benjamin Marzinski
2007-09-13 16:36 ` Benjamin Marzinski
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Marzinski @ 2007-09-13 16:30 UTC (permalink / raw)
To: cluster-devel.redhat.com
gfs2 was deadlocking because most code paths acquire the sd_log_flush_lock and
then the page locks, while code paths going through gfs2_writepage acquire the
page locks and then the log flush lock.
This patch makes gfs2_writepage do a trylock on the sd_log_flush_lock. If it
fails, writepage simply redirtys the page and gives up. This should keep the
deadlock from happening, however it might cause a performance hit.
Signed-off-by: Benjamin E. Marzinski <bmarzins@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Cluster-devel] [PATCH] [GFS2] bz253089 Make gfs2_writepage use trylock on the log_flush lock
2007-09-13 16:30 [Cluster-devel] [PATCH] [GFS2] bz253089 Make gfs2_writepage use trylock on the log_flush lock Benjamin Marzinski
@ 2007-09-13 16:36 ` Benjamin Marzinski
2007-09-14 16:50 ` Steven Whitehouse
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Marzinski @ 2007-09-13 16:36 UTC (permalink / raw)
To: cluster-devel.redhat.com
On Thu, Sep 13, 2007 at 11:30:37AM -0500, Benjamin Marzinski wrote:
> gfs2 was deadlocking because most code paths acquire the sd_log_flush_lock and
> then the page locks, while code paths going through gfs2_writepage acquire the
> page locks and then the log flush lock.
>
> This patch makes gfs2_writepage do a trylock on the sd_log_flush_lock. If it
> fails, writepage simply redirtys the page and gives up. This should keep the
> deadlock from happening, however it might cause a performance hit.
>
> Signed-off-by: Benjamin E. Marzinski <bmarzins@redhat.com>
And here is the actual patch
-------------- next part --------------
diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/log.c linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/log.c
--- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/log.c 2007-09-05 09:33:11.000000000 -0500
+++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/log.c 2007-09-11 10:03:48.000000000 -0500
@@ -288,7 +288,7 @@ static void ail2_empty(struct gfs2_sbd *
* Returns: errno
*/
-int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks)
+int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks, int wait)
{
unsigned int try = 0;
unsigned reserved_blks = 6 * (4096 / sdp->sd_vfs->s_blocksize);
@@ -301,6 +301,10 @@ int gfs2_log_reserve(struct gfs2_sbd *sd
gfs2_log_lock(sdp);
while(sdp->sd_log_blks_free <= (blks + reserved_blks)) {
gfs2_log_unlock(sdp);
+ if (!wait) {
+ mutex_unlock(&sdp->sd_log_reserve_mutex);
+ return -EBUSY;
+ }
gfs2_ail1_empty(sdp, 0);
gfs2_log_flush(sdp, NULL);
@@ -310,10 +314,21 @@ int gfs2_log_reserve(struct gfs2_sbd *sd
}
sdp->sd_log_blks_free -= blks;
gfs2_log_unlock(sdp);
- mutex_unlock(&sdp->sd_log_reserve_mutex);
-
- down_read(&sdp->sd_log_flush_lock);
+ if (wait){
+ mutex_unlock(&sdp->sd_log_reserve_mutex);
+ down_read(&sdp->sd_log_flush_lock);
+ }
+ else {
+ if (!down_read_trylock(&sdp->sd_log_flush_lock)) {
+ gfs2_log_lock(sdp);
+ sdp->sd_log_blks_free += blks;
+ gfs2_log_unlock(sdp);
+ mutex_unlock(&sdp->sd_log_reserve_mutex);
+ return -EBUSY;
+ }
+ mutex_unlock(&sdp->sd_log_reserve_mutex);
+ }
return 0;
}
diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/log.h linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/log.h
--- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/log.h 2007-08-29 14:11:37.000000000 -0500
+++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/log.h 2007-09-09 08:18:08.000000000 -0500
@@ -50,7 +50,7 @@ unsigned int gfs2_struct2blk(struct gfs2
int gfs2_ail1_empty(struct gfs2_sbd *sdp, int flags);
-int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks);
+int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks, int wait);
void gfs2_log_release(struct gfs2_sbd *sdp, unsigned int blks);
struct buffer_head *gfs2_log_get_buf(struct gfs2_sbd *sdp);
diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/ops_address.c linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/ops_address.c
--- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/ops_address.c 2007-08-29 14:11:37.000000000 -0500
+++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/ops_address.c 2007-09-09 08:20:50.000000000 -0500
@@ -140,7 +140,7 @@ static int gfs2_writepage(struct page *p
if ((sdp->sd_args.ar_data == GFS2_DATA_ORDERED || gfs2_is_jdata(ip)) &&
PageChecked(page)) {
ClearPageChecked(page);
- error = gfs2_trans_begin(sdp, RES_DINODE + 1, 0);
+ error = gfs2_do_trans_begin(sdp, RES_DINODE + 1, 0, 0);
if (error)
goto out_ignore;
if (!page_has_buffers(page)) {
diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/trans.c linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/trans.c
--- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/trans.c 2007-09-04 07:45:25.000000000 -0500
+++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/trans.c 2007-09-09 09:23:05.000000000 -0500
@@ -25,8 +25,8 @@
#include "trans.h"
#include "util.h"
-int gfs2_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
- unsigned int revokes)
+int gfs2_do_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
+ unsigned int revokes, int wait)
{
struct gfs2_trans *tr;
int error;
@@ -61,7 +61,7 @@ int gfs2_trans_begin(struct gfs2_sbd *sd
goto fail_gunlock;
}
- error = gfs2_log_reserve(sdp, tr->tr_reserved);
+ error = gfs2_log_reserve(sdp, tr->tr_reserved, wait);
if (error)
goto fail_gunlock;
diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/trans.h linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/trans.h
--- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/trans.h 2007-09-04 07:45:25.000000000 -0500
+++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/trans.h 2007-09-09 08:21:29.000000000 -0500
@@ -25,8 +25,8 @@ struct gfs2_glock;
#define RES_STATFS 1
#define RES_QUOTA 2
-int gfs2_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
- unsigned int revokes);
+int gfs2_do_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
+ unsigned int revokes, int wait);
void gfs2_trans_end(struct gfs2_sbd *sdp);
@@ -36,4 +36,11 @@ void gfs2_trans_add_revoke(struct gfs2_s
void gfs2_trans_add_unrevoke(struct gfs2_sbd *sdp, u64 blkno);
void gfs2_trans_add_rg(struct gfs2_rgrpd *rgd);
+
+static inline int gfs2_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
+ unsigned int revokes)
+{
+ return gfs2_do_trans_begin(sdp, blocks, revokes, 1);
+}
+
#endif /* __TRANS_DOT_H__ */
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Cluster-devel] [PATCH] [GFS2] bz253089 Make gfs2_writepage use trylock on the log_flush lock
2007-09-13 16:36 ` Benjamin Marzinski
@ 2007-09-14 16:50 ` Steven Whitehouse
0 siblings, 0 replies; 3+ messages in thread
From: Steven Whitehouse @ 2007-09-14 16:50 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hi,
On Thu, 2007-09-13 at 11:36 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 13, 2007 at 11:30:37AM -0500, Benjamin Marzinski wrote:
> > gfs2 was deadlocking because most code paths acquire the sd_log_flush_lock and
> > then the page locks, while code paths going through gfs2_writepage acquire the
> > page locks and then the log flush lock.
> >
> > This patch makes gfs2_writepage do a trylock on the sd_log_flush_lock. If it
> > fails, writepage simply redirtys the page and gives up. This should keep the
> > deadlock from happening, however it might cause a performance hit.
> >
> > Signed-off-by: Benjamin E. Marzinski <bmarzins@redhat.com>
>
> And here is the actual patch
>
Now in the -nmw git tree. Thanks,
Steve.
> plain text document attachment (253089_try4.patch)
> diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/log.c linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/log.c
> --- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/log.c 2007-09-05 09:33:11.000000000 -0500
> +++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/log.c 2007-09-11 10:03:48.000000000 -0500
> @@ -288,7 +288,7 @@ static void ail2_empty(struct gfs2_sbd *
> * Returns: errno
> */
>
> -int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks)
> +int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks, int wait)
> {
> unsigned int try = 0;
> unsigned reserved_blks = 6 * (4096 / sdp->sd_vfs->s_blocksize);
> @@ -301,6 +301,10 @@ int gfs2_log_reserve(struct gfs2_sbd *sd
> gfs2_log_lock(sdp);
> while(sdp->sd_log_blks_free <= (blks + reserved_blks)) {
> gfs2_log_unlock(sdp);
> + if (!wait) {
> + mutex_unlock(&sdp->sd_log_reserve_mutex);
> + return -EBUSY;
> + }
> gfs2_ail1_empty(sdp, 0);
> gfs2_log_flush(sdp, NULL);
>
> @@ -310,10 +314,21 @@ int gfs2_log_reserve(struct gfs2_sbd *sd
> }
> sdp->sd_log_blks_free -= blks;
> gfs2_log_unlock(sdp);
> - mutex_unlock(&sdp->sd_log_reserve_mutex);
> -
> - down_read(&sdp->sd_log_flush_lock);
>
> + if (wait){
> + mutex_unlock(&sdp->sd_log_reserve_mutex);
> + down_read(&sdp->sd_log_flush_lock);
> + }
> + else {
> + if (!down_read_trylock(&sdp->sd_log_flush_lock)) {
> + gfs2_log_lock(sdp);
> + sdp->sd_log_blks_free += blks;
> + gfs2_log_unlock(sdp);
> + mutex_unlock(&sdp->sd_log_reserve_mutex);
> + return -EBUSY;
> + }
> + mutex_unlock(&sdp->sd_log_reserve_mutex);
> + }
> return 0;
> }
>
> diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/log.h linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/log.h
> --- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/log.h 2007-08-29 14:11:37.000000000 -0500
> +++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/log.h 2007-09-09 08:18:08.000000000 -0500
> @@ -50,7 +50,7 @@ unsigned int gfs2_struct2blk(struct gfs2
>
> int gfs2_ail1_empty(struct gfs2_sbd *sdp, int flags);
>
> -int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks);
> +int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks, int wait);
> void gfs2_log_release(struct gfs2_sbd *sdp, unsigned int blks);
>
> struct buffer_head *gfs2_log_get_buf(struct gfs2_sbd *sdp);
> diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/ops_address.c linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/ops_address.c
> --- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/ops_address.c 2007-08-29 14:11:37.000000000 -0500
> +++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/ops_address.c 2007-09-09 08:20:50.000000000 -0500
> @@ -140,7 +140,7 @@ static int gfs2_writepage(struct page *p
> if ((sdp->sd_args.ar_data == GFS2_DATA_ORDERED || gfs2_is_jdata(ip)) &&
> PageChecked(page)) {
> ClearPageChecked(page);
> - error = gfs2_trans_begin(sdp, RES_DINODE + 1, 0);
> + error = gfs2_do_trans_begin(sdp, RES_DINODE + 1, 0, 0);
> if (error)
> goto out_ignore;
> if (!page_has_buffers(page)) {
> diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/trans.c linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/trans.c
> --- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/trans.c 2007-09-04 07:45:25.000000000 -0500
> +++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/trans.c 2007-09-09 09:23:05.000000000 -0500
> @@ -25,8 +25,8 @@
> #include "trans.h"
> #include "util.h"
>
> -int gfs2_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
> - unsigned int revokes)
> +int gfs2_do_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
> + unsigned int revokes, int wait)
> {
> struct gfs2_trans *tr;
> int error;
> @@ -61,7 +61,7 @@ int gfs2_trans_begin(struct gfs2_sbd *sd
> goto fail_gunlock;
> }
>
> - error = gfs2_log_reserve(sdp, tr->tr_reserved);
> + error = gfs2_log_reserve(sdp, tr->tr_reserved, wait);
> if (error)
> goto fail_gunlock;
>
> diff -urpN --exclude-from=linux-2.6.18-44.gfs2abhi.003_test_clean/Documentation/dontdiff linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/trans.h linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/trans.h
> --- linux-2.6.18-44.gfs2abhi.003_test_clean/fs/gfs2/trans.h 2007-09-04 07:45:25.000000000 -0500
> +++ linux-2.6.18-44.gfs2abhi.003_test/fs/gfs2/trans.h 2007-09-09 08:21:29.000000000 -0500
> @@ -25,8 +25,8 @@ struct gfs2_glock;
> #define RES_STATFS 1
> #define RES_QUOTA 2
>
> -int gfs2_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
> - unsigned int revokes);
> +int gfs2_do_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
> + unsigned int revokes, int wait);
>
> void gfs2_trans_end(struct gfs2_sbd *sdp);
>
> @@ -36,4 +36,11 @@ void gfs2_trans_add_revoke(struct gfs2_s
> void gfs2_trans_add_unrevoke(struct gfs2_sbd *sdp, u64 blkno);
> void gfs2_trans_add_rg(struct gfs2_rgrpd *rgd);
>
> +
> +static inline int gfs2_trans_begin(struct gfs2_sbd *sdp, unsigned int blocks,
> + unsigned int revokes)
> +{
> + return gfs2_do_trans_begin(sdp, blocks, revokes, 1);
> +}
> +
> #endif /* __TRANS_DOT_H__ */
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-09-14 16:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-09-13 16:30 [Cluster-devel] [PATCH] [GFS2] bz253089 Make gfs2_writepage use trylock on the log_flush lock Benjamin Marzinski
2007-09-13 16:36 ` Benjamin Marzinski
2007-09-14 16:50 ` 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).