[Cluster-devel] [PATCH v2 2/3] GFS2: Implement iomap for block_map

[Steven Whitehouse <swhiteho@redhat.com>]

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