Date:	Thu, 10 Oct 2013 15:56:40 +0900
From:	Namjae Jeon <linkinjeon@...il.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	viro@...iv.linux.org.uk, mtk.manpages@...il.com, tytso@....edu,
	adilger.kernel@...ger.ca, bpm@....com, elder@...nel.org,
	hch@...radead.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-ext4@...r.kernel.org,
	xfs@....sgi.com, a.sangwan@...sung.com,
	Namjae Jeon <namjae.jeon@...sung.com>
Subject: Re: [PATCH RESEND 1/7] fs: add new flag(FALLOC_FL_COLLAPSE_RANGE) for fallocate

Hi Dave.

> >
> > +     /*
> > +      * Collapse range works only on fs block size aligned offsets.
> > +      * Check if collapse range is contained within (aligned)i_size.
> > +      * Collapse range can only be used exclusively.
> > +      */
> > +     if ((mode & FALLOC_FL_COLLAPSE_RANGE) &&
> > +         (offset & blksize_mask || len & blksize_mask ||
> > +          mode & ~FALLOC_FL_COLLAPSE_RANGE ||
> > +          (offset + len >
> > +           round_up(i_size_read(inode), (blksize_mask + 1)))))
> > +             return -EINVAL;
>
> There's lots of individual checks here. Let's separate them out
> logically. Firstly, "Collapse range can only be used exclusively" is
> a mode parameter check, and so should be done directly after
> validating the mode only contains known commands. i.e. in the first
> hunk above.
>
> Secondly, "Collapse range works only on fs block size aligned
> offsets" is an implementation constraint, not an API constraint.
> i.e. There is no reason why a filesystem can't implement byte range
> granularity for this operation, it just may not be efficient for all
> fielsystems and so they don't choose to implement byte range
> granularity. Further, filesystems might have different allocation
> constraints than the filesystem block size (think bigalloc on ext4,
> per-file extent size hints for XFS), and these generally aren't
> reflected in inode->i_blkbits. In these cases, the granularity of
> the collapse operation can only be determined by the filesystem
> itself, not this high level code.
>
> Hence I think the granularity check should be put into a helper
> function that the filesystem's ->fallocate() method calls if it can
> only support fs block aligned operations. That allows each
> filesystem to determine it's own constraints on a per-operation
> basis.
>
> All that remains here is the "within file size"
> check, and that doesn't need to be rounded up to block size to check
> if it is valid. If the range given overlaps the end of file in any
> way, then it is a truncate operation....

Okay, I will update your points on next version patch.
>
>
> > diff --git a/include/uapi/linux/falloc.h b/include/uapi/linux/falloc.h
> > index 990c4cc..9614b72 100644
> > --- a/include/uapi/linux/falloc.h
> > +++ b/include/uapi/linux/falloc.h
> > @@ -4,6 +4,23 @@
> >  #define FALLOC_FL_KEEP_SIZE  0x01 /* default is extend size */
> >  #define FALLOC_FL_PUNCH_HOLE 0x02 /* de-allocates range */
> >  #define FALLOC_FL_NO_HIDE_STALE      0x04 /* reserved codepoint */
> > +/*
> > + * FALLOC_FL_COLLAPSE_RANGE:
> > + * This flag works in 2 steps.
> > + * Firstly, it deallocates any data blocks present between [offset,
> > offset+len)
> > + * This step is same as punch hole and leaves a hole in the place from
> > where
> > + * the blocks are removed.
> > + * Next, it eliminates the hole created by moving data blocks into it.
> > + * For extent based file systems, we achieve this functionality simply
> > by
> > + * updating the starting logical offset of each extent which appears
> > beyond
> > + * the hole. As this flag works on blocks of filesystem, the offset and
> > len
> > + * provided to fallocate should be aligned with block size of
> > filesystem.
>
> Hmmm - you're describing an implementation, not the API. i.e. what
> you need to describe is the functionality users are provided with by
> the flag and it's usage constraints, not how filesystems need to
> implement it. Something like:
Okay, I will reference your shared description.

>
> "FALLOC_FL_COLLAPSE_RANGE is used to remove a range of a file
> without leaving a hole in the file. The contents of the file beyond
> the range being removed is appended to the start offset of the range
> being removed (i.e. the hole that was punched is "collapsed"),
> resulting in a file layout that looks like the range that was
> removed never existed. As suchm collapsing a range of a file changes
> the size of the file, reducing it by the same length of the range
> that has been removed by the operation.
>
> Different filesystems may implement different limitations on the
> granularity of the operation. Most will limit operations to
> filesystem block size boundaries, but this boundary may be larger or
> smaller depending on the filesystem and/or the configuration of the
> filesystem or file.
>
> Attempting to collapse a range that crosses the end of the file is
> considered an illegal operation - just use ftruncate(2) if you need
> to collapse a range that crosses EOF."
>
> > +#define FALLOC_FL_COLLAPSE_RANGE     0x08 /* it does not leave a hole
> > */
>
> With the large descriptive comment, there is no need for the
> appended "/* it does not leave a hole */" comment.
Okay. will remove.

Thanks for review.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists