From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Thu, 3 Nov 2016 09:45:47 +0000 Subject: [Cluster-devel] [PATCH v2 2/3] GFS2: Implement iomap for block_map In-Reply-To: <20161102210158.GE14023@dastard> References: <1477682971-4517-1-git-send-email-rpeterso@redhat.com> <1477682971-4517-3-git-send-email-rpeterso@redhat.com> <58146ADD.9010702@redhat.com> <20161031200716.GM22126@dastard> <5819B3BC.6080602@redhat.com> <20161102210158.GE14023@dastard> Message-ID: <581B074B.4000301@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Hi, On 02/11/16 21:01, Dave Chinner wrote: > On Wed, Nov 02, 2016 at 09:37:00AM +0000, Steven Whitehouse wrote: >> Hi, >> >> On 31/10/16 20:07, Dave Chinner wrote: >>> On Sat, Oct 29, 2016 at 10:24:45AM +0100, Steven Whitehouse wrote: >>>> On 28/10/16 20:29, Bob Peterson wrote: >>>>> + if (create) >>>>> + flags |= IOMAP_WRITE; >>>> Hmm, I wonder why IOMAP_WRITE and IOMAP_ZERO are separate flags from >>>> the iomap.flags field... Christoph, was there a specific reason for >>>> that? >>> They are different actions. IOMAP_WRITE requires allocation over >>> holes and conversion of unwritten extents to allow writing of >>> user data into the range. IOMAP_ZERO is for zeroing a range of a >>> file via iomap_zero_range() and it does not require allocation - it >>> skips holes and unwritten regions as they are already guaranteed to >>> contain zeros. >>> >>> One *could* allocate blocks with IOMAP_ZERO if desired (i.e. >>> implement IOMAP_ZERO as though it implied IOMAP_WRITE) and >>> iomap_zero_range_actor() will zero the allocated regions >>> appropriately, but it's not necessary to do if it is already known >>> what ranges of the file contain zeros... >>> >>> Cheers, >>> >>> Dave. >> That wasn't quite what I was getting at... we have two sets of >> flags. IOMAP_ZERO, IOMAP_WRITE and IOMAP_REPORT form one set that >> are passed as an argument to ->iomap_begin() and ->iomap_end() > Yes, those are mapping operation control flags that determine the > mapping operation to be done. > >> whereas we also have IOMAP_F_* which are set in the iomap.flags > And those are per-map state flags that apply to the extent being > returned. > >> field. I guess perhaps the former is intended as the input flags to >> the functions, where as the latter are output flags from >> ->iomap_begin()? > Yes, I thought that was obvious - it didn't occur to me that there'd > be any confusion there. What can we add to the iomap.h definitions > to make this clearer? Yes, probably more to do with me not having as much time to look at the code as I used to :( We could maybe update the comment in iomap.h next to iomap.flags to say /* Flags set in ->iomap_begin() */ The other thought is to rename the flags argument to ->iomap_begin and ->iomap_end so that we don't land up with two things both called flags, but I'm not sure what a better name would be ... "inflags" or "action" or something like that? Could the iomap that is passed to ->iomap_end() possibly become const? I guess that the only thing that might need to be updated by ->iomap_end() is the iomap.flags, but if it truely is readonly for ->iomap_end() then perhaps it could be const in that case. There may well be better solutions here too, so very open to better suggestions. For GFS2 then we want to use the IOMAP_* flags as they are at the moment, but for the function that maps back from iomap to gfs2_block_map() we'd need to add a IOMAP_F_ZEROED or something like that from which to set the BH_zeronew() flag, which is used as both a request and result flag for the zeroing operation. That flag would very likely go away in due course when the final iomap migration is done for GFS2, so would just be a temporary thing in the mean time. >> In that case I would expect that ->iomap_end() >> would view the iomap.flags as read only. > iomap.flags, like the rest of the struct iomap that is returned from > ->iomap_begin(), is readonly for all users. Only the filesystem > itself can change extent mappings or state..... > > Cheers, > > Dave. Yes - and in this initial GFS2 case things are a bit odd in that we don't have a full iomap_begin() / iomap_end() implementation yet as would be used for "normal buffered i/o" but only a subset of that functionality which is being used for FIEMAP, while at the same time retaining backwards compatibility with gfs2_block_map(). The intent is eventually to do the mapping with iomap of course, but that will be a later patch set. I also wondered whether it would be possible to write a generic implementation of SEEK_HOLE/SEEK_END for iomap supporting filesystems at the VFS level. That would get GFS2 support for those two operations and be a nice clean up too. I've added an item on our todo list for that, but it may be a little while before we get around to it, Steve