Re: [PATCH v2 1/6] vfs: Add iomap_seek_hole_data helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux