On Tue, 7 May 2013 13:38:35 -0700, Andrew Morton wrote: > On Wed, 01 May 2013 16:39:30 +0900 (JST) Ryusuke Konishi > <[email protected]> wrote: > >> DESCRIPTION: >> There are use-cases when NILFS2 file system (formatted with block size >> lesser than 4 KB) can be remounted in RO mode because of encountering >> of "broken bmap" issue. >> >> ... >> >> --- a/fs/nilfs2/inode.c >> +++ b/fs/nilfs2/inode.c >> @@ -219,13 +219,25 @@ static int nilfs_writepage(struct page *page, struct >> writeback_control *wbc) >> >> static int nilfs_set_page_dirty(struct page *page) >> { >> - int ret = __set_page_dirty_buffers(page); >> + int ret = __set_page_dirty_nobuffers(page); >> >> - if (ret) { >> + if (page_has_buffers(page)) { >> struct inode *inode = page->mapping->host; >> - unsigned nr_dirty = 1 << (PAGE_SHIFT - inode->i_blkbits); >> + unsigned nr_dirty = 0; >> + struct buffer_head *bh, *head; >> >> - nilfs_set_file_dirty(inode, nr_dirty); >> + bh = head = page_buffers(page); >> + do { >> + /* Do not mark hole blocks dirty */ >> + if (!buffer_dirty(bh) || !buffer_mapped(bh)) >> + continue; >> + >> + set_buffer_dirty(bh); > > This doesn't seem right. The only time we will run set_buffer_dirty() > is if the buffer was dirty and mapped. What's the point in running > set_buffer_dirty() against an already dirty buffer? > > I suspect you intended (buffer_dirty || !buffer_mapped)
Yes, sorry, this is my mistake. I'll correct it and redo tests. >> + nr_dirty++; >> + } while (bh = bh->b_this_page, bh != head); >> + >> + if (nr_dirty) >> + nilfs_set_file_dirty(inode, nr_dirty); > > Rather secondarily: the above code appears to assume that no other > thread can come in and mark a buffer dirty while the loop is executing. > This *should* be OK - the page is locked (or it should be). Can you > confirm that this is the case? There are no other places where a > buffer can be concurrently dirtied against an unlocked page? Yes, I confirmed no other thread concurrently marks the above buffer dirty. The nilfs_page_set_dirty function is called only for data blocks of regular files, directory data, or symlinks. And, nilfs only dirties this type of buffers through routines in fs/buffer.c. Every routine in fs/buffer.c that can be called from nilfs, was protecting call sites of mark_buffer_dirty() by page lock. I'll add a comment to brief this assumption. Thanks, Ryusuke Konishi >> } >> return ret; >> } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
