diff mbox

[v2,1/6] vfs: Add iomap_seek_hole_data helper

Message ID 1498224884-346-2-git-send-email-agruenba@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andreas Gruenbacher June 23, 2017, 1:34 p.m. UTC 
Filesystems can use this for implementing lseek SEEK_HOLE / SEEK_DATA
support via iomap.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/iomap.c            | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/iomap.h |  3 ++
 2 files changed, 92 insertions(+)

Comments

kernel test robot June 25, 2017, 5 a.m. UTC | #1 
Hi Andreas,

[auto build test ERROR on gfs2/for-next]
[also build test ERROR on v4.12-rc6 next-20170623]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andreas-Gruenbacher/SEEK_HOLE-SEEK_DATA-via-iomap/20170625-115825
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git for-next
config: x86_64-randconfig-x015-201726 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   fs/iomap.c: In function 'iomap_seek_hole_actor':
>> fs/iomap.c:603:11: error: implicit declaration of function 'page_cache_seek_hole_data' [-Werror=implicit-function-declaration]
     offset = page_cache_seek_hole_data(inode, offset, length, SEEK_HOLE);
              ^~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/page_cache_seek_hole_data +603 fs/iomap.c

   597	{
   598		if (iomap->type == IOMAP_HOLE)
   599			goto found;
   600		length = iomap->offset + iomap->length - offset;
   601		if (iomap->type != IOMAP_UNWRITTEN)
   602			return length;
 > 603		offset = page_cache_seek_hole_data(inode, offset, length, SEEK_HOLE);
   604		if (offset < 0)
   605			return length;
   606	found:

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Christoph Hellwig June 26, 2017, 10:47 a.m. UTC | #2 
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@redhat.com>
> ---
>  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?

> +	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;
	}

> +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.
Andreas Gruenbacher June 26, 2017, 2:11 p.m. UTC | #3 
On Mon, Jun 26, 2017 at 12:47 PM, Christoph Hellwig <hch@infradead.org> 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@redhat.com>
>> ---
>>  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
diff mbox

Patch

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;
+	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;
+}
+
+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;
+}
+
+/*
+ * Filesystem helper for lseek SEEK_HOLE / SEEK_DATA.
+ */
+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 *);
+	loff_t size = i_size_read(inode);
+	loff_t length;
+
+	/* Nothing to be found beyond the end of the file. */
+	if (offset >= size)
+		return -ENXIO;
+	length = size - offset;
+
+	switch(whence) {
+	case SEEK_HOLE:
+		actor = iomap_seek_hole_actor;
+		break;
+
+	case SEEK_DATA:
+		actor = iomap_seek_data_actor;
+		break;
+	}
+
+	while (length > 0) {
+		loff_t ret;
+
+		ret = iomap_apply(inode, offset, length, IOMAP_REPORT, ops,
+				  &offset, actor);
+		if (ret <= 0) {
+			if (ret < 0)
+				return ret;
+			break;
+		}
+		offset += ret;
+		length -= ret;
+	}
+
+	if (length <= 0) {
+		/* There is an implicit hole at the end of the file. */
+		if (whence != SEEK_HOLE)
+			return -ENXIO;
+
+		/* The last segment can extend beyond the end of the file. */
+		if (offset > size)
+			offset = size;
+	}
+
+	return offset;
+}
+EXPORT_SYMBOL_GPL(iomap_seek_hole_data);
+
 /*
  * Private flags for iomap_dio, must not overlap with the public ones in
  * iomap.h:
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index f753e78..ff89026 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -83,6 +83,9 @@  int iomap_truncate_page(struct inode *inode, loff_t pos, bool *did_zero,
 int iomap_page_mkwrite(struct vm_fault *vmf, const struct iomap_ops *ops);
 int iomap_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		loff_t start, loff_t len, const struct iomap_ops *ops);
+loff_t iomap_seek_hole_data(struct inode *inode, loff_t offset,
+			    int whence, const struct iomap_ops *ops);
+
 
 /*
  * Flags for direct I/O ->end_io: