On Fri, Nov 28, 2008 at 6:18 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> One minor thing -- you could do the !PageUptodate check first? If the
>> page is already uptodate, then everything is much simpler I think? (and
>> PageChecked should not be set).
>>
>> if (!PageUptodate(page)) {
>> if (PageChecked(page)) {
>> if (copied == len)
>> SetPageUptodate(page);
>> ClearPageChecked(page);
>> } else if (copied == PAGE_CACHE_SIZE)
>> SetPageUptodate(page);
>> }
>>
>> I don't know if you think that's better or not, but I really like to
>> make it clear that this is the !PageUptodate logic, and we never try
>> to SetPageUptodate on an already uptodate page.
>>
>> But I guess it is just a matter of style. So go with whatever you like
>> best.
> --------------[snip]---------------
> Subject: [PATCH] cifs: clean up conditionals in cifs_write_end
>
> Make it clear that the conditionals at the start of cifs_write_end are
> just for the situation when the page is not uptodate.
>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> fs/cifs/file.c | 12 +++++++-----
> 1 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index f0a81e6..202a20f 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1475,12 +1475,14 @@ 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 (PageChecked(page)) {
> - if (copied == len)
> + if (!PageUptodate(page)) {
> + if (PageChecked(page)) {
> + if (copied == len)
> + SetPageUptodate(page);
> + ClearPageChecked(page);
> + } else if (copied == PAGE_CACHE_SIZE)
> SetPageUptodate(page);
> - ClearPageChecked(page);
> - } else if (!PageUptodate(page) && copied == PAGE_CACHE_SIZE)
> - SetPageUptodate(page);
> + }
>
> if (!PageUptodate(page)) {
> char *page_data;
Jeff and I just talked about his patch above, and decided not to make
his minor change above. Moving PageUptodate check earlier would
complicate things in one way ... if PageChecked were ever set at the
same time as PageUptodate then PageChecked would stay set. That is
probably not an issue but that is clearer with the original.
--
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
