On Fri, 22 Dec 2006, Jean Delvare wrote:
>
> The approach seems quite broken to me, the users should have been fixed
> _before_ removing the function, so as to avoid compilation failures.
> These are a pain for testers, and break git bisect too. Grmbl.
This needed to be fixed, and quite frankly, things don't get fixed nearly
as quickly if you don't just break them first. And there really were just
two filesystems that got broken, cifs being one of them.
I just can't test it.
> Now that it's done... Steve, can you please take a look and provide a
> patch so that cifs builds again?
CIFS _should_ be using "clear_page_dirty_for_io()" in that place, and that
will fix the build. However, the reason I didn't just do that myself is
that I can't test the end result, and for the life of me, I can't see
where CIFS does the "end_page_writeback()" that it needs to do at IO
completion time.
And the thing that confuses me about that, is that if CIFS doesn't do
"end_page_writeback()", then it was already broken before - because when
the VM calls "->writepage()" the clear_page_dirty_for_io() will have been
done by the VM, and it needs that "end_page_writeback()" so that the
system can know when the IO is done.
I _suspect_ that those "unlock_page()" calls should be accompanied by a
"end_page_writeback()" call, and that the proper patch MAY look something
like the appended, but I worry about having missed something really
subtle. Maybe there's a end_page_writeback() somewhere else.
And if there isn't, I wonder if shared mappings have _ever_ worked on
CIFS? And if so, how? That writeback bit thing isn't new per se.
So this may or may not fix it. If you can test it (_including_ with some
dirty shared mmap-on-mmap action, please - just call me kinky), I'll
commit it. But I need somebody who actually uses this to test it.
Linus
---
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 0f05cab..4f0472d 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1245,7 +1245,7 @@ retry:
wait_on_page_writeback(page);
if (PageWriteback(page) ||
- !test_clear_page_dirty(page)) {
+ !clear_page_dirty_for_io(page)) {
unlock_page(page);
break;
}
@@ -1253,6 +1253,7 @@ retry:
if (page_offset(page) >= mapping->host->i_size) {
done = 1;
unlock_page(page);
+ end_page_writeback(page);
break;
}
@@ -1316,6 +1317,7 @@ retry:
SetPageError(page);
kunmap(page);
unlock_page(page);
+ end_page_writeback(page);
page_cache_release(page);
}
if ((wbc->nr_to_write -= n_iov) <= 0)
@@ -1356,7 +1358,8 @@ static int cifs_writepage(struct page* page, struct writeback_control *wbc)
rc = cifs_partialpagewrite(page, 0, PAGE_CACHE_SIZE);
SetPageUptodate(page); /* BB add check for error and Clearuptodate? */
unlock_page(page);
- page_cache_release(page);
+ end_page_writeback(page);
+ page_cache_release(page);
FreeXid(xid);
return rc;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
