badly clear PG_Writeback bit in ll_ap_completion can produce false

positive assertion

Branch b1_8
b=14742
i=nikita
i=johan

diff --git a/lustre/ChangeLog b/lustre/ChangeLog
index 5a9ea4d..f4c25b4 100644 (file)
--- a/lustre/ChangeLog
+++ b/lustre/ChangeLog
@@ -24,6 +24,13 @@ tbd Sun Microsystems, Inc.
                'tunefs.lustre --param="mdt.quota_type=ug1" $MDTDEV'.
          For more information, please refer to bugzilla 13904.
 
+Severity   : normal
+Bugzilla   : 14742
+Frequency  : rare
+Description: ASSERTION(CheckWriteback(page,cmd)) failed
+Details    : badly clear PG_Writeback bit in ll_ap_completion can produce false
+             positive assertion.
+
 Severity   : enhancement
 Bugzilla   : 15865
 Description: Update to RHEL5 kernel-2.6.18-53.1.21.el5.
@@ -42,8 +49,8 @@ Details    : Need properly lock accesses the flock deadlock detection list.
 Severity   : minor
 Bugzilla   : 15837
 Description: oops in page fault handler
-Details    : kernel page fault handler can return two special 'pages' in error case, don't
-             try dereference NOPAGE_SIGBUS and NOPAGE_OMM.
+Details    : kernel page fault handler can return two special 'pages' in 
+             error case, don't try dereference NOPAGE_SIGBUS and NOPAGE_OMM.
 
 Severity   : minor
 Bugzilla   : 15716

diff --git a/lustre/include/linux/lustre_compat25.h b/lustre/include/linux/lustre_compat25.h
index 861d95f..d9e4499 100644 (file)
--- a/lustre/include/linux/lustre_compat25.h
+++ b/lustre/include/linux/lustre_compat25.h
@@ -220,7 +220,8 @@ extern void __d_move(struct dentry *dentry, struct dentry *target);
 #endif
 
 #define CheckWriteback(page, cmd) \
-        (!(!PageWriteback(page) && cmd == OBD_BRW_WRITE))
+        ((!PageWriteback(page) && (cmd & OBD_BRW_READ)) || \
+         (PageWriteback(page) && (cmd & OBD_BRW_WRITE)))
 
 #else /* 2.4.. */
 

diff --git a/lustre/llite/rw.c b/lustre/llite/rw.c
index 2e6a71f..fffec34 100644 (file)
--- a/lustre/llite/rw.c
+++ b/lustre/llite/rw.c
@@ -308,6 +308,14 @@ int ll_prepare_write(struct file *file, struct page *page, unsigned from,
         return rc;
 }
 
+/**
+ * make page ready for ASYNC write
+ * \param data - pointer to llap cookie
+ * \param cmd - is OBD_BRW_* macroses
+ *
+ * \retval 0 is page successfully prepared to send
+ * \retval -EAGAIN is page not need to send
+ */
 static int ll_ap_make_ready(void *data, int cmd)
 {
         struct ll_async_page *llap;
@@ -324,7 +332,9 @@ static int ll_ap_make_ready(void *data, int cmd)
         if (TryLockPage(page))
                 RETURN(-EAGAIN);
 
-        LASSERT(!PageWriteback(page));
+        LASSERTF(!(cmd & OBD_BRW_READ) || !PageWriteback(page),
+                "cmd %x page %p ino %lu index %lu fl %lx\n", cmd, page,
+                 page->mapping->host->i_ino, page->index, page->flags);
 
         /* if we left PageDirty we might get another writepage call
          * in the future.  list walkers are bright enough
@@ -333,16 +343,13 @@ static int ll_ap_make_ready(void *data, int cmd)
          * we got the page cache list we'd create a lock inversion
          * with the removepage path which gets the page lock then the
          * cli lock */
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
-        clear_page_dirty(page);
-#else
-        LASSERTF(!PageWriteback(page),"cmd %x page %p ino %lu index %lu\n", cmd, page,
-                 page->mapping->host->i_ino, page->index);
-        clear_page_dirty_for_io(page);
+        if(!clear_page_dirty_for_io(page)) {
+               unlock_page(page);
+               RETURN(-EAGAIN);
+       }
 
         /* This actually clears the dirty bit in the radix tree.*/
         set_page_writeback(page);
-#endif
 
         LL_CDEBUG_PAGE(D_PAGE, page, "made ready\n");
         page_cache_get(page);
@@ -772,9 +779,8 @@ static int queue_or_sync_write(struct obd_export *exp, struct inode *inode,
 
         if (!rc && async_flags & ASYNC_READY) {
                 unlock_page(llap->llap_page);
-                if (PageWriteback(llap->llap_page)) {
+                if (PageWriteback(llap->llap_page))
                         end_page_writeback(llap->llap_page);
-                }
         }
 
         LL_CDEBUG_PAGE(D_PAGE, llap->llap_page, "sync write returned %d\n", rc);
@@ -927,6 +933,14 @@ int ll_ap_completion(void *data, int cmd, struct obdo *oa, int rc)
 #endif
         }
 
+        /* be carefull about clear WB.
+         * if WB will cleared after page lock is released - paralel IO can be
+         * started before ap_make_ready is finished - so we will be have page
+         * with PG_Writeback set from ->writepage() and completed READ which
+         * clear this flag */
+        if ((cmd & OBD_BRW_WRITE) && PageWriteback(page))
+                end_page_writeback(page);
+
         unlock_page(page);
 
         if (cmd & OBD_BRW_WRITE) {
@@ -934,9 +948,6 @@ int ll_ap_completion(void *data, int cmd, struct obdo *oa, int rc)
                 ll_try_done_writing(page->mapping->host);
         }
 
-        if (PageWriteback(page)) {
-                end_page_writeback(page);
-        }
         page_cache_release(page);
 
         RETURN(ret);
@@ -1745,19 +1756,22 @@ int ll_writepage(struct page *page)
                 rc = queue_or_sync_write(exp, inode, llap, CFS_PAGE_SIZE,
                                          ASYNC_READY | ASYNC_URGENT);
         }
-        if (rc)
-                page_cache_release(page);
-out:
         if (rc) {
-                if (!lli->lli_async_rc)
-                        lli->lli_async_rc = rc;
                 /* re-dirty page on error so it retries write */
-                if (PageWriteback(page)) {
+                if (PageWriteback(page))
                         end_page_writeback(page);
-                }
+
                 /* resend page only for not started IO*/
                 if (!PageError(page))
                         ll_redirty_page(page);
+
+                page_cache_release(page);
+        }
+out:
+        if (rc) {
+                if (!lli->lli_async_rc)
+                        lli->lli_async_rc = rc;
+                /* resend page only for not started IO*/
                 unlock_page(page);
         }
         RETURN(rc);

diff --git a/lustre/osc/osc_request.c b/lustre/osc/osc_request.c
index c2f25ee..841c9e6 100644 (file)
--- a/lustre/osc/osc_request.c
+++ b/lustre/osc/osc_request.c
@@ -2080,6 +2080,17 @@ out:
 
 /* the loi lock is held across this function but it's allowed to release
  * and reacquire it during its work */
+/**
+ * prepare pages for ASYNC io and put pages in send queue.
+ *
+ * \param cli -
+ * \param loi -
+ * \param cmd - OBD_BRW_* macroses
+ * \param lop - pending pages
+ *
+ * \return zero if pages successfully add to send queue.
+ * \return not zere if error occurring.
+ */
 static int osc_send_oap_rpc(struct client_obd *cli, struct lov_oinfo *loi,
                             int cmd, struct loi_oap_pages *lop)
 {
@@ -2153,12 +2164,14 @@ static int osc_send_oap_rpc(struct client_obd *cli, struct lov_oinfo *loi,
                 /*
                  * Page submitted for IO has to be locked. Either by
                  * ->ap_make_ready() or by higher layers.
-                 *
-                 * XXX nikita: this assertion should be adjusted when lustre
-                 * starts using PG_writeback for pages being written out.
                  */
 #if defined(__KERNEL__) && defined(__linux__)
-                LASSERT(PageLocked(oap->oap_page));
+                 if(!(PageLocked(oap->oap_page) &&
+                     (CheckWriteback(oap->oap_page, cmd) || oap->oap_oig !=NULL))) {
+                       CDEBUG(D_PAGE, "page %p lost wb %lx/%x\n",
+                               oap->oap_page, (long)oap->oap_page->flags, oap->oap_async_flags);
+                        LBUG();
+                }
 #endif
                 /* If there is a gap at the start of this page, it can't merge
                  * with any previous page, so we'll hand the network a