Looks fine to me, you can add in the acked-by ... if Dave or Nick
agree and it tests out ok then I will push to cifs-2.6.git within next
day or so.
On Sun, Nov 23, 2008 at 5:57 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Sat, 22 Nov 2008 14:27:44 -0600
> Dave Kleikamp <shaggy@xxxxxxxxxxxxxxxxxx> wrote:
>
>> Just took a real quick look, but I have one comment. I'd leave the page
>> alone if it's already PageUptodate. I wouldn't bother setting
>> PageChecked and let cifs_write_end leave it alone when its already
>> uptodate.
>
> Ok, it makes sense not to flip page bits unless we need to. How's this
> instead? It also switches the code to use zero_user_segments() instead
> of calling kmap/memset directly.
>
> I've dropped Steve's SoB for now since this has morphed quite a bit from
> his original patch. Steve, feel free to add it back if you approve.
>
> --------------------[snip]--------------------
>
> From da222d5125ec4095f10e9a4c984b6bd6b0cacf18 Mon Sep 17 00:00:00 2001
> From: Jeff Layton <jlayton@xxxxxxxxxx>
> Date: Sun, 23 Nov 2008 06:54:42 -0500
> Subject: [PATCH] cifs: fix regression in cifs_write_begin/cifs_write_end
>
> The conversion to write_begin/write_end interfaces had a bug where we
> were passing a bad parameter to cifs_readpage_worker. Rather than
> passing the page offset of the start of the write, we needed to pass the
> offset of the beginning of the page. This was reliably showing up as
> data corruption in the fsx-linux test from LTP.
>
> It also became evident that this code was occasionally doing unnecessary
> read calls. Optimize those away by using the PG_checked flag to indicate
> that the unwritten part of the page has been initialized.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/cifs/file.c | 68 +++++++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index b691b89..9114ea5 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1475,7 +1475,10 @@ static int cifs_write_end(struct file *file, struct address_space *mapping,
> cFYI(1, ("write_end for page %p from pos %lld with %d bytes",
> page, pos, copied));
>
> - if (!PageUptodate(page) && copied == PAGE_CACHE_SIZE)
> + if (PageChecked(page)) {
> + SetPageUptodate(page);
> + ClearPageChecked(page);
> + } else if (!PageUptodate(page) && copied == PAGE_CACHE_SIZE)
> SetPageUptodate(page);
>
> if (!PageUptodate(page)) {
> @@ -2062,39 +2065,68 @@ static int cifs_write_begin(struct file *file, struct address_space *mapping,
> {
> pgoff_t index = pos >> PAGE_CACHE_SHIFT;
> loff_t offset = pos & (PAGE_CACHE_SIZE - 1);
> + loff_t page_start = pos & PAGE_MASK;
> + loff_t i_size;
> + struct inode *inode;
> + struct page *page;
> + int rc = 0;
>
> cFYI(1, ("write_begin from %lld len %d", (long long)pos, len));
>
> - *pagep = __grab_cache_page(mapping, index);
> - if (!*pagep)
> - return -ENOMEM;
> + page = __grab_cache_page(mapping, index);
> + if (!page) {
> + rc = -ENOMEM;
> + goto out;
> + }
>
> - if (PageUptodate(*pagep))
> - return 0;
> + if (PageUptodate(page))
> + goto out;
>
> /* If we are writing a full page it will be up to date,
> no need to read from the server */
> if (len == PAGE_CACHE_SIZE && flags & AOP_FLAG_UNINTERRUPTIBLE)
> - return 0;
> -
> - if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> - int rc;
> + goto out;
>
> - /* might as well read a page, it is fast enough */
> - rc = cifs_readpage_worker(file, *pagep, &offset);
> + /*
> + * optimize away the read when we have an oplock, and we're not
> + * expecting to use any of the data we'd be reading in. That is, when
> + * the page lies beyond the EOF, or straddles the EOF and the write
> + * would cover all of the existing data in the file.
> + */
> + inode = page->mapping->host;
> + if (CIFS_I(inode)->clientCanCacheRead) {
> + i_size = i_size_read(inode);
> + if (page_start >= i_size ||
> + (offset == 0 && (pos + len) >= i_size)) {
> + zero_user_segments(page, 0, offset, offset + len,
> + PAGE_CACHE_SIZE);
> + /*
> + * PageChecked means that the parts of the page to
> + * which we're not writing are considered up to date.
> + * Once the write data is copied to the page, it can
> + * be set uptodate.
> + */
> + SetPageChecked(page);
> + goto out;
> + }
> + }
>
> - /* we do not need to pass errors back
> - e.g. if we do not have read access to the file
> - because cifs_write_end will attempt synchronous writes
> - -- shaggy */
> + if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> + /*
> + * might as well read a page, it is fast enough. If we get
> + * an error, we don't need to return it. cifs_write_end will
> + * do a sync write instead since PG_uptodate isn't set.
> + */
> + cifs_readpage_worker(file, page, &page_start);
> } else {
> /* we could try using another file handle if there is one -
> but how would we lock it to prevent close of that handle
> racing with this read? In any case
> this will be written out by write_end so is fine */
> }
> -
> - return 0;
> +out:
> + *pagep = page;
> + return rc;
> }
>
> const struct address_space_operations cifs_addr_ops = {
> --
> 1.5.5.1
>
>
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
