[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Cluster-devel] [GFS2] Missing bit of readpages() fix



On Thu, 2006-12-14 at 18:43 +0000, Steven Whitehouse wrote:
> Hi,
> 
> On Thu, 2006-12-14 at 12:34 -0600, Russell Cattelan wrote:
> > On Thu, 2006-12-14 at 18:09 +0000, Steven Whitehouse wrote:
> > > Hi,
> > > 
> > > The patch below is the missing bit of the readpages fix which hasn't yet
> > > made it upstream. Its derived from Russell's original patch with a
> > > slight change in that I've fixed up the original comment at the top of
> > > the function rather than adding one in the body of the function. I'll
> > > commit this tomorrow if there are no objections & also add it to the
> > > pending RHEL5 patches,
> > ya don't move the comment, it doesn't make much sense when it is moved
> > out of context.
> > 
> Well we need to fix the comment at the top anyway, otherwise it would be
> wrong and even more confusing, there didn't seem to be any reason to
> mention it again since the patch removes most of the intervening lines.
> 
> > I'm still having trouble understanding why this patch is getting chopped
> > up and rewritten? Doing so is significantly confusing things.
> > 
> What else is left to go upstream? As far as I know this is all there is
> left for this particular fix as we have the rest of it already...
Upstream looked strange: here is a version of the patch against
upstream.
(Comments fixed up)
> 
> Steve.
> 
> > > 
> > > Steve.
> > > 
> > > diff --git a/fs/gfs2/ops_address.c b/fs/gfs2/ops_address.c
> > > index d8d69a7..6903153 100644
> > > --- a/fs/gfs2/ops_address.c
> > > +++ b/fs/gfs2/ops_address.c
> > > @@ -256,7 +256,7 @@ out_unlock:
> > >   *    the page lock and the glock) and return having done no I/O. Its
> > >   *    obviously not something we'd want to do on too regular a basis.
> > >   *    Any I/O we ignore at this time will be done via readpage later.
> > > - * 2. We have to handle stuffed files here too.
> > > + * 2. We don't handle stuffed files here we let readpage do the honours.
> > >   * 3. mpage_readpages() does most of the heavy lifting in the common case.
> > >   * 4. gfs2_get_block() is relied upon to set BH_Boundary in the right places.
> > >   * 5. We use LM_FLAG_TRY_1CB here, effectively we then have lock-ahead as
> > > @@ -269,8 +269,7 @@ static int gfs2_readpages(struct file *f
> > >  	struct gfs2_inode *ip = GFS2_I(inode);
> > >  	struct gfs2_sbd *sdp = GFS2_SB(inode);
> > >  	struct gfs2_holder gh;
> > > -	unsigned page_idx;
> > > -	int ret;
> > > +	int ret = 0;
> > >  	int do_unlock = 0;
> > >  
> > >  	if (likely(file != &gfs2_internal_file_sentinel)) {
> > > @@ -289,29 +288,8 @@ static int gfs2_readpages(struct file *f
> > >  			goto out_unlock;
> > >  	}
> > >  skip_lock:
> > > -	if (gfs2_is_stuffed(ip)) {
> > > -		struct pagevec lru_pvec;
> > > -		pagevec_init(&lru_pvec, 0);
> > > -		for (page_idx = 0; page_idx < nr_pages; page_idx++) {
> > > -			struct page *page = list_entry(pages->prev, struct page, lru);
> > > -			prefetchw(&page->flags);
> > > -			list_del(&page->lru);
> > > -			if (!add_to_page_cache(page, mapping,
> > > -					       page->index, GFP_KERNEL)) {
> > > -				ret = stuffed_readpage(ip, page);
> > > -				unlock_page(page);
> > > -				if (!pagevec_add(&lru_pvec, page))
> > > -					 __pagevec_lru_add(&lru_pvec);
> > > -			} else {
> > > -				page_cache_release(page);
> > > -			}
> > > -		}
> > > -		pagevec_lru_add(&lru_pvec);
> > > -		ret = 0;
> > > -	} else {
> > > -		/* What we really want to do .... */
> > > +	if (!gfs2_is_stuffed(ip))
> > >  		ret = mpage_readpages(mapping, pages, nr_pages, gfs2_get_block);
> > > -	}
> > >  
> > >  	if (do_unlock) {
> > >  		gfs2_glock_dq_m(1, &gh);
> > > 
-- 
Russell Cattelan <cattelan redhat com>
Fix readpage to return AOP_TRUNCATED_PAGE
if the glock is already held.

Fix readpages to simple return when the inode
is stuffed, the vfs will then call readpage
which can correctly handle the stuffed case.

The problem with trying to handle the stuffed
case:  nr_pages can be > 0 which can happen
when the file is being read from and truncated
at the same time.

Signed-off-by: Russell Cattelan <cattelan redhat com> 
Index: gfs2_crap/fs/gfs2/ops_address.c
===================================================================
--- gfs2_crap.orig/fs/gfs2/ops_address.c	2006-12-14 13:30:43.000000000 -0600
+++ gfs2_crap/fs/gfs2/ops_address.c	2006-12-14 13:44:02.476372398 -0600
@@ -205,7 +205,7 @@ static int gfs2_readpage(struct file *fi
 	struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host);
 	struct gfs2_file *gf = NULL;
 	struct gfs2_holder gh;
-	int error;
+	int ret;
 	int do_unlock = 0;
 
 	if (likely(file != &gfs2_internal_file_sentinel)) {
@@ -215,36 +215,46 @@ static int gfs2_readpage(struct file *fi
 				/* gfs2_sharewrite_nopage has grabbed the ip->i_gl already */
 				goto skip_lock;
 		}
-		gfs2_holder_init(ip->i_gl, LM_ST_SHARED, GL_ATIME|LM_FLAG_TRY_1CB, &gh);
+		gfs2_holder_init(ip->i_gl, LM_ST_SHARED,
+				 LM_FLAG_TRY_1CB|GL_ATIME, &gh);
 		do_unlock = 1;
-		error = gfs2_glock_nq_atime(&gh);
-		if (unlikely(error))
+		ret = gfs2_glock_nq_m_atime(1, &gh);
+		/* if we couldn't get glock just fail readpage
+		 * otherwise we deadlock with other ops (i.e. truncate)
+		 * on page lock
+		 */
+		if (ret == GLR_TRYFAILED) {
+			ret = AOP_TRUNCATED_PAGE;
+			goto out_unlock;
+		}
+		if (unlikely(ret)) {
 			goto out_unlock;
+		}
 	}
 
 skip_lock:
 	if (gfs2_is_stuffed(ip)) {
-		error = stuffed_readpage(ip, page);
+		ret = stuffed_readpage(ip, page);
 		unlock_page(page);
 	} else
-		error = mpage_readpage(page, gfs2_get_block);
+		ret = mpage_readpage(page, gfs2_get_block);
 
 	if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
-		error = -EIO;
+		ret = -EIO;
 
 	if (do_unlock) {
 		gfs2_glock_dq_m(1, &gh);
 		gfs2_holder_uninit(&gh);
 	}
-out:
-	return error;
+	goto out;
+
 out_unlock:
-	if (error == GLR_TRYFAILED)
-		error = AOP_TRUNCATED_PAGE;
 	unlock_page(page);
 	if (do_unlock)
 		gfs2_holder_uninit(&gh);
-	goto out;
+
+out:
+	return ret;
 }
 
 /**
@@ -256,10 +266,9 @@ out_unlock:
  *    the page lock and the glock) and return having done no I/O. Its
  *    obviously not something we'd want to do on too regular a basis.
  *    Any I/O we ignore at this time will be done via readpage later.
- * 2. We have to handle stuffed files here too.
- * 3. mpage_readpages() does most of the heavy lifting in the common case.
- * 4. gfs2_get_block() is relied upon to set BH_Boundary in the right places.
- * 5. We use LM_FLAG_TRY_1CB here, effectively we then have lock-ahead as
+ * 2. mpage_readpages() does most of the heavy lifting in the common case.
+ * 3. gfs2_get_block() is relied upon to set BH_Boundary in the right places.
+ * 4. We use LM_FLAG_TRY_1CB here, effectively we then have lock-ahead as
  *    well as read-ahead.
  */
 static int gfs2_readpages(struct file *file, struct address_space *mapping,
@@ -270,8 +279,9 @@ static int gfs2_readpages(struct file *f
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
 	struct gfs2_holder gh;
 	unsigned page_idx;
-	int ret;
+	int ret = 0;
 	int do_unlock = 0;
+	int do_un_atime = 0;
 
 	if (likely(file != &gfs2_internal_file_sentinel)) {
 		if (file) {
@@ -282,51 +292,38 @@ static int gfs2_readpages(struct file *f
 		gfs2_holder_init(ip->i_gl, LM_ST_SHARED,
 				 LM_FLAG_TRY_1CB|GL_ATIME, &gh);
 		do_unlock = 1;
-		ret = gfs2_glock_nq_atime(&gh);
-		if (ret == GLR_TRYFAILED)
-			goto out_noerror;
-		if (unlikely(ret))
-			goto out_unlock;
+
+		ret = gfs2_glock_nq_m_atime(1, &gh);
+		if (unlikely(ret)) {
+			ret = 0;
+			goto out;
+		}
+		do_un_atime = 1;
 	}
 skip_lock:
 	if (gfs2_is_stuffed(ip)) {
-		struct pagevec lru_pvec;
-		pagevec_init(&lru_pvec, 0);
-		for (page_idx = 0; page_idx < nr_pages; page_idx++) {
-			struct page *page = list_entry(pages->prev, struct page, lru);
-			prefetchw(&page->flags);
-			list_del(&page->lru);
-			if (!add_to_page_cache(page, mapping,
-					       page->index, GFP_KERNEL)) {
-				ret = stuffed_readpage(ip, page);
-				unlock_page(page);
-				if (!pagevec_add(&lru_pvec, page))
-					 __pagevec_lru_add(&lru_pvec);
-			} else {
-				page_cache_release(page);
-			}
-		}
-		pagevec_lru_add(&lru_pvec);
+		/*
+		 * here due to a race with a truncate
+		 * simply fail and let readpage handle the stuffed case
+		 */
 		ret = 0;
-	} else {
-		/* What we really want to do .... */
-		ret = mpage_readpages(mapping, pages, nr_pages, gfs2_get_block);
+		goto out;
 	}
 
-	if (do_unlock) {
+	/* What we really want to do .... */
+	ret = mpage_readpages(mapping, pages, nr_pages, gfs2_get_block);
+
+out:
+	if (do_un_atime)
 		gfs2_glock_dq_m(1, &gh);
+
+	if (do_unlock)
 		gfs2_holder_uninit(&gh);
-	}
-out:
+
 	if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
 		ret = -EIO;
+
 	return ret;
-out_noerror:
-	ret = 0;
-out_unlock:
-	if (do_unlock)
-		gfs2_holder_uninit(&gh);
-	goto out;
 }
 
 /**

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]