On Mon, Jun 26, 2017 at 12:47 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> On Fri, Jun 23, 2017 at 03:34:39PM +0200, Andreas Gruenbacher wrote:
>> Filesystems can use this for implementing lseek SEEK_HOLE / SEEK_DATA
>> support via iomap.
>>
>> Signed-off-by: Andreas Gruenbacher <agruenba@xxxxxxxxxx>
>> ---
>> fs/iomap.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/iomap.h | 3 ++
>> 2 files changed, 92 insertions(+)
>>
>> diff --git a/fs/iomap.c b/fs/iomap.c
>> index 4b10892..781f0a0 100644
>> --- a/fs/iomap.c
>> +++ b/fs/iomap.c
>> @@ -584,6 +584,95 @@ int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fi,
>> }
>> EXPORT_SYMBOL_GPL(iomap_fiemap);
>>
>> +static loff_t
>> +iomap_seek_hole_actor(struct inode *inode, loff_t offset, loff_t length,
>> + void *data, struct iomap *iomap)
>> +{
>> + if (iomap->type == IOMAP_HOLE)
>> + goto found;
>> + length = iomap->offset + iomap->length - offset;
>
> What is the problem with the passed in length value?
Yep, no need to recompute that.
>> + if (iomap->type != IOMAP_UNWRITTEN)
>> + return length;
>> + offset = page_cache_seek_hole_data(inode, offset, length, SEEK_HOLE);
>> + if (offset < 0)
>> + return length;
>> +found:
>> + *(loff_t *)data = offset;
>> + return 0;
>
> What about using a switch statement?
>
> switch (iomap->type) {
> case IOMAP_UNWRITTEN:
> offset = page_cache_seek_hole_data(inode, offset, length,
> SEEK_HOLE);
> if (offset < 0)
> return length;
> /*FALLTHRU*/
> case IOMAP_HOLE:
> *(loff_t *)data = offset;
> return 0;
> default:
> return length;
> }
Ok.
>> +static loff_t
>> +iomap_seek_data_actor(struct inode *inode, loff_t offset, loff_t length,
>> + void *data, struct iomap *iomap)
>> +{
>> + if (iomap->type != IOMAP_HOLE && iomap->type != IOMAP_UNWRITTEN)
>> + goto found;
>> + length = iomap->offset + iomap->length - offset;
>> + if (iomap->type != IOMAP_UNWRITTEN)
>> + return length;
>> + offset = page_cache_seek_hole_data(inode, offset, length, SEEK_DATA);
>> + if (offset < 0)
>> + return length;
>> +found:
>> + *(loff_t *)data = offset;
>> + return 0;
>
>> +loff_t
>> +iomap_seek_hole_data(struct inode *inode, loff_t offset,
>> + int whence, const struct iomap_ops *ops)
>> +{
>> + static loff_t (*actor)(struct inode *, loff_t, loff_t, void *,
>> + struct iomap *);
>
> I wonder (but I'm not sure) if it would be simpler and easier to
> understand to just have to different functions for SEEK_HOLE
> vs SEEK_DATA here.
Neither variant seems obviously better to me.
Thanks,
Andreas
