On Wed, 01 May 2013 16:39:30 +0900 (JST) Ryusuke Konishi <konishi.ryusuke@xxxxxxxxxxxxx> 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)
> + 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?
> }
> return ret;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
