All of lore.kernel.org
 help / color / mirror / Atom feed
From: Boaz Harrosh <ooo@electrozaur.com>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Trond Myklebust <trond.myklebust@primarydata.com>,
	Christoph Lameter <cl@linux.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nfs@vger.kernel.org, linux-mm@kvack.org,
	osd-dev@open-osd.org
Subject: Re: [PATCH] osd fs: __r4w_get_page rely on PageUptodate for uptodate
Date: Mon, 02 Nov 2015 13:05:32 +0200	[thread overview]
Message-ID: <5637437C.4070306@electrozaur.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1511011513240.11427@eggly.anvils>

On 11/02/2015 01:39 AM, Hugh Dickins wrote:
<>
>> This patch is not correct!
> 
> I think you have actually confirmed that the patch is correct:
> why bother to test PageDirty or PageWriteback when PageUptodate
> already tells you what you need?
> 
> Or do these filesystems do something unusual with PageUptodate
> when PageDirty is set?  I didn't find it.
> 

This is kind of delicate stuff. It took me a while to get it right
when I did it. I don't remember all the details.

But consider this option:

exofs_write_begin on a full PAGE_CACHE_SIZE, the page is instantiated
new in page-cache is that PageUptodate(page) then? I thought not.
(exofs does not set that)

Now that page I do not want to read in. The latest data is in memory.
(Same when this page is in writeback, dirty-bit is cleared)

So for sure if page is dirty or writeback then we surly do not need a read.
only if not then we need to consider the  PageUptodate(page) state.

Do you think the code is actually wrong as is?

BTW: Very similar code is in fs/nfs/objlayout/objio_osd.c::__r4w_get_page

> Thanks,
> Hugh
> 
<>

Thanks
Boaz


WARNING: multiple messages have this Message-ID (diff)
From: Boaz Harrosh <ooo@electrozaur.com>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Trond Myklebust <trond.myklebust@primarydata.com>,
	Christoph Lameter <cl@linux.com>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-nfs@vger.kernel.org, linux-mm@kvack.org,
	osd-dev@open-osd.org
Subject: Re: [PATCH] osd fs: __r4w_get_page rely on PageUptodate for uptodate
Date: Mon, 02 Nov 2015 13:05:32 +0200	[thread overview]
Message-ID: <5637437C.4070306@electrozaur.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1511011513240.11427@eggly.anvils>

On 11/02/2015 01:39 AM, Hugh Dickins wrote:
<>
>> This patch is not correct!
> 
> I think you have actually confirmed that the patch is correct:
> why bother to test PageDirty or PageWriteback when PageUptodate
> already tells you what you need?
> 
> Or do these filesystems do something unusual with PageUptodate
> when PageDirty is set?  I didn't find it.
> 

This is kind of delicate stuff. It took me a while to get it right
when I did it. I don't remember all the details.

But consider this option:

exofs_write_begin on a full PAGE_CACHE_SIZE, the page is instantiated
new in page-cache is that PageUptodate(page) then? I thought not.
(exofs does not set that)

Now that page I do not want to read in. The latest data is in memory.
(Same when this page is in writeback, dirty-bit is cleared)

So for sure if page is dirty or writeback then we surly do not need a read.
only if not then we need to consider the  PageUptodate(page) state.

Do you think the code is actually wrong as is?

BTW: Very similar code is in fs/nfs/objlayout/objio_osd.c::__r4w_get_page

> Thanks,
> Hugh
> 
<>

Thanks
Boaz

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2015-11-02 11:05 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-29 18:43 [PATCH] osd fs: __r4w_get_page rely on PageUptodate for uptodate Hugh Dickins
2015-10-29 18:43 ` Hugh Dickins
2015-11-01 10:00 ` Boaz Harrosh
2015-11-01 10:00   ` Boaz Harrosh
2015-11-01 23:39   ` Hugh Dickins
2015-11-01 23:39     ` Hugh Dickins
2015-11-01 23:39     ` Hugh Dickins
2015-11-02 11:05     ` Boaz Harrosh [this message]
2015-11-02 11:05       ` Boaz Harrosh
2015-11-03  2:49       ` Hugh Dickins
2015-11-03  2:49         ` Hugh Dickins
2015-11-03  9:24         ` Boaz Harrosh
2015-11-03  9:24           ` Boaz Harrosh
2015-11-03 15:39           ` Hugh Dickins
2015-11-03 15:39             ` Hugh Dickins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5637437C.4070306@electrozaur.com \
    --to=ooo@electrozaur.com \
    --cc=akpm@linux-foundation.org \
    --cc=cl@linux.com \
    --cc=hughd@google.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=osd-dev@open-osd.org \
    --cc=trond.myklebust@primarydata.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.