From 50eea5b5111497b848bcdba71f6e7c64193090f6 Mon Sep 17 00:00:00 2001 From: shadow Date: Mon, 30 Jun 2008 12:20:20 +0000 Subject: [PATCH] badly clear PG_Writeback bit in ll_ap_completion can produce false positive assertion Branch b1_8 b=14742 i=nikita i=johan --- lustre/ChangeLog | 11 +++++-- lustre/include/linux/lustre_compat25.h | 3 +- lustre/llite/rw.c | 54 +++++++++++++++++++++------------- lustre/osc/osc_request.c | 21 ++++++++++--- 4 files changed, 62 insertions(+), 27 deletions(-) diff --git a/lustre/ChangeLog b/lustre/ChangeLog index 5a9ea4d..f4c25b4 100644 --- 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 --- 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 --- 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 --- 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 -- 1.8.3.1