Re: [PATCH 5/9] fs: Move start and length fiemap fields into fiemap_extent_info

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

 



On Fri, Aug 02, 2019 at 11:51:16AM +0200, Carlos Maiolino wrote:
> > >  
> > >  STATIC int
> > >  xfs_vn_fiemap(
> > > -	struct inode		*inode,
> > > -	struct fiemap_extent_info *fieinfo,
> > > -	u64			start,
> > > -	u64			length)
> > > +	struct inode		  *inode,
> > > +	struct fiemap_extent_info *fieinfo)
> > >  {
> > > -	int			error;
> > > +	u64	start = fieinfo->fi_start;
> > > +	u64	length = fieinfo->fi_len;
> > > +	int	error;
> > 
> > Would be nice if the variable name indentation was consistent here, but
> > otherwise the xfs part looks ok.
> 
> These fields are removed on the next patch, updating it is really required?

Yes, please.

> > 
> > >  
> > >  	xfs_ilock(XFS_I(inode), XFS_IOLOCK_SHARED);
> > >  	if (fieinfo->fi_flags & FIEMAP_FLAG_XATTR) {
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index d5e7c744aea6..7b744b7de24e 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -1705,11 +1705,14 @@ extern bool may_open_dev(const struct path *path);
> > >   * VFS FS_IOC_FIEMAP helper definitions.
> > >   */
> > >  struct fiemap_extent_info {
> > > -	unsigned int fi_flags;		/* Flags as passed from user */
> > > -	unsigned int fi_extents_mapped;	/* Number of mapped extents */
> > > -	unsigned int fi_extents_max;	/* Size of fiemap_extent array */
> > > -	struct fiemap_extent __user *fi_extents_start; /* Start of
> > > -							fiemap_extent array */
> > > +	unsigned int	fi_flags;		/* Flags as passed from user */
> > > +	u64		fi_start;
> > > +	u64		fi_len;
> > 
> > Comments for these two new fields?
> 
> Sure, how about this:
> 
>        u64           fi_start;            /* Logical offset at which
>                                              start mapping */
>        u64           fi_len;              /* Logical length of mapping
>                                              the caller cares about */
> 
> 
> btw, Above indentation won't match final result

Looks good to me.

--D

> 
> Christoph, may I keep your reviewed tag by updating the comments as above?
> Otherwise I'll just remove your tag
> 
> > 
> > --D
> > 
> > > +	unsigned int	fi_extents_mapped;	/* Number of mapped extents */
> > > +	unsigned int	fi_extents_max;		/* Size of fiemap_extent array */
> > > +	struct		fiemap_extent __user *fi_extents_start;	/* Start of
> > > +								   fiemap_extent
> > > +								   array */
> > >  };
> > >  int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
> > >  			    u64 phys, u64 len, u32 flags);
> > > @@ -1841,8 +1844,7 @@ struct inode_operations {
> > >  	int (*setattr) (struct dentry *, struct iattr *);
> > >  	int (*getattr) (const struct path *, struct kstat *, u32, unsigned int);
> > >  	ssize_t (*listxattr) (struct dentry *, char *, size_t);
> > > -	int (*fiemap)(struct inode *, struct fiemap_extent_info *, u64 start,
> > > -		      u64 len);
> > > +	int (*fiemap)(struct inode *, struct fiemap_extent_info *);
> > >  	int (*update_time)(struct inode *, struct timespec64 *, int);
> > >  	int (*atomic_open)(struct inode *, struct dentry *,
> > >  			   struct file *, unsigned open_flag,
> > > @@ -3199,11 +3201,10 @@ extern int vfs_readlink(struct dentry *, char __user *, int);
> > >  
> > >  extern int __generic_block_fiemap(struct inode *inode,
> > >  				  struct fiemap_extent_info *fieinfo,
> > > -				  loff_t start, loff_t len,
> > >  				  get_block_t *get_block);
> > >  extern int generic_block_fiemap(struct inode *inode,
> > > -				struct fiemap_extent_info *fieinfo, u64 start,
> > > -				u64 len, get_block_t *get_block);
> > > +				struct fiemap_extent_info *fieinfo,
> > > +				get_block_t *get_block);
> > >  
> > >  extern struct file_system_type *get_filesystem(struct file_system_type *fs);
> > >  extern void put_filesystem(struct file_system_type *fs);
> > > -- 
> > > 2.20.1
> > > 
> 
> -- 
> Carlos



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux