Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752495Ab1DRFcO (ORCPT ); Mon, 18 Apr 2011 01:32:14 -0400 Received: from mail-iy0-f174.google.com ([209.85.210.174]:61133 "EHLO mail-iy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751432Ab1DRFcK convert rfc822-to-8bit (ORCPT ); Mon, 18 Apr 2011 01:32:10 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=skvXFu9hRqapVTtOJsc8e47gR6CNLOnHubeb7N0dsJbXeFzsANsoIMIC3TD3Vd0Hps XxNg+QAS723P/hZX9BeqbrK44hWNMdSWJzITLAg76KrRDt2hwybd5jchkNPObSn/hxmH yzG3MdsgxI6ZHHy8qcOexT0gG5LT84R4FwPUc= MIME-Version: 1.0 In-Reply-To: <040a7fa3-14dd-4960-a296-cfdd061e015f@default> References: <83ef8b69-f041-43e6-a5a9-880ff3da26f2@default> <871v135xvj.fsf@devron.myhome.or.jp> <040a7fa3-14dd-4960-a296-cfdd061e015f@default> Date: Mon, 18 Apr 2011 14:32:08 +0900 Message-ID: Subject: Re: [PATCH V8 4/8] mm/fs: add hooks to support cleancache From: Minchan Kim To: Dan Magenheimer Cc: OGAWA Hirofumi , Andrew Morton , Chris Mason , viro@zeniv.linux.org.uk, adilger.kernel@dilger.ca, tytso@mit.edu, mfasheh@suse.com, jlbec@evilplan.org, matthew@wil.cx, linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, ocfs2-devel@oss.oracle.com, linux-mm@kvack.org, hch@infradead.org, ngupta@vflare.org, jeremy@goop.org, JBeulich@novell.com, Kurt Hackel , npiggin@kernel.dk, Dave Mccracken , riel@redhat.com, avi@redhat.com, Konrad Wilk , mel@csn.ul.ie, yinghan@google.com, gthelen@google.com, torvalds@linux-foundation.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7302 Lines: 172 On Sat, Apr 16, 2011 at 3:53 AM, Dan Magenheimer wrote: >> From: OGAWA Hirofumi [mailto:hirofumi@mail.parknet.co.jp] >> >> Andrew Morton writes: >> >> >> > Before I suggested a thing about cleancache_flush_page, >> >> > cleancache_flush_inode. >> >> > >> >> > what's the meaning of flush's semantic? >> >> > I thought it means invalidation. >> >> > AFAIC, how about change flush with invalidate? >> >> >> >> I'm not sure the words "flush" and "invalidate" are defined >> >> precisely or used consistently everywhere in computer >> >> science, but I think that "invalidate" is to destroy >> >> a "pointer" to some data, but not necessarily destroy the >> >> data itself.   And "flush" means to actually remove >> >> the data.  So one would "invalidate a mapping" but one >> >> would "flush a cache". >> >> >> >> Since cleancache_flush_page and cleancache_flush_inode >> >> semantically remove data from cleancache, I think flush >> >> is a better name than invalidate. >> >> >> >> Does that make sense? >> > >> > nope ;) >> > >> > Kernel code freely uses "flush" to refer to both invalidation and to >> > writeback, sometimes in confusing ways.  In this case, >> > cleancache_flush_inode and cleancache_flush_page rather sound like >> they >> > might write those things to backing store. >> >> I'd like to mention about *_{get,put}_page too. In linux get/put is not >> meaning read/write. There is {get,put}_page those are refcount stuff >> (Yeah, and I felt those methods does refcount by quick read. But it >> seems to be false. There is no xen codes, so I don't know actually >> though.). >> >> And I agree, I also think the needing thing is consistency on the linux >> codes (term). >> >> Thanks. >> -- >> OGAWA Hirofumi > > Hmmm, yes, that's a point of confusion also.  No, cleancache put/get > do not have any relationship with reference counting. > > Andrew, I wonder if you would be so kind as to read the following > and make a "ruling".  If you determine a preferable set of names, > I will abide by your decision and repost (if necessary). > > The problem is this: The English language has a limited number > of words that can be used to represent data motion and mapping > and most/all of them are already used in the kernel, often, > to quote Andrew, "in confusing ways."  Complicating this, I > think the semantics of the cleancache operations are different > from the semantics of any other kernel operation... intentionally > so, because the value of cleancache is a direct result of those > differing semantics.  And the cleancache semantics > are fairly complex (again intentionally) so a single function > name can't possibly describe the semantics. > > The cleancache operations are: > - put (page) > - get (page) > - flush page > - flush inode > - init fs > - flush fs > > I think these names are reasonable representations of the > semantics of the operations performed... but I'm not a kernel > expert so there is certainly room for disagreement.  Though I > absolutely recognize the importance of a "name", I am primarily > interested in merging the semantics of the operations and > would happily accept any name that kernel developers could > agree on.  However, I fear that there will be NO name that > will satisfy all, so would prefer to keep the existing names. > If some renaming is eventually agreed upon, this could be done > post-merge. > > Here's a brief description of the semantics: > > The cleancache operation currently known as "put" has the > following semantics:  If *possible*, please take the data > contained in the pageframe referred to by this struct page > into cleancache and associate it with the filesystem-determined > "handle" derived from the struct page. > > The cleancache operation currently known as "get" has the > following semantics:  Derive the filesystem-determined handle > from this struct page.  If cleancache contains a page matching > that handle, recreate the page of data from cleancache and > place the results in the pageframe referred to by the > struct page.  Then delete in cleancache any record of the > handle and any data associated with it, so that a > subsequent "get" will no longer find a match for the handle; > any space used for the data can also be freed. > > (Note that "take the data" and "recreate the page of data" are > similar in semantics to "copy to" and "copy from", but since > the cleancache operation may perform an "inflight" transformation > on the data, and "copy" usually means a byte-for-byte replication, > the word "copy" is also misleading.) > > The cleancache operation currently known as "flush" has the > following semantics:  Derive the filesystem-determined handle > from this struct page and struct mapping.  If cleancache > contains a page matching that handle, delete in cleancache any > record of the handle and any data associated with it, so that a > subsequent "get" will no longer find a match for the handle; > any space used for the data can also be freed > > The cleancache operation currently known as "flush inode" has > the following semantics: Derive the filesystem-determined filekey > from this struct mapping.  If cleancache contains ANY handles > matching that filekey, delete in cleancache any record of > any matching handle and any data associated with those handles; > any space used for the data can also be freed. > > The cleancache operation currently known as "init fs" has > the following semantics: Create a unique poolid to refer > to this filesystem and save it in the superblock's > cleancache_poolid field. > > The cleancache operation currently known as "flush fs" has > the following semantics: Get the cleancache_poolid field > from this superblock.  If cleancache contains ANY handles > associated with that poolid, delete in cleancache any > record of any matching handles and any data associated with > those handles; any space used for the data can also be freed. > Also, set the superblock's cleancache_poolid to be invalid > and, in cleancache, recycle the poolid so a subsequent init_fs > operation can reuse it. > > That's all! > > Thanks, > Dan > At least, I didn't confused your semantics except just flush. That's why I suggested only flush but after seeing your explaining, there is another thing I want to change. The get/put is common semantic of reference counting in kernel but in POV your semantics, it makes sense to me but get has a exclusive semantic so I want to represent it with API name. Maybe cleancache_get_page_exclusive. The summary is that I don't want to change all API name. Just two thing. (I am not sure you and others agree on me. It's just suggestion). 1. cleancache_flush_page -> cleancache_[invalidate|remove]_page 2. cleancache_get_page -> cleancache_get_page_exclusive BTW, Nice description. Please include it in documentation if we can't reach the conclusion. It will help others to understand semantic of cleancache. Thanks, Dan. -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/