On Wed, Jul 01, 2015 at 11:58:43AM +1000, Dave Chinner wrote:
> On Thu, Jun 25, 2015 at 04:40:16PM -0700, Darrick J. Wong wrote:
> > Gate all the reflink functions (which generally involve an expensive
> > trip to the reflink btree) on an inode flag which is applied to both
> > inodes at reflink time. This minimizes reflink's impact on non-CoW
> > files.
>
> Ah, I see you do this reflink inode flag here. This should be one of
> the first patches, not the last. i.e. the patch series should
> build up all the supporting infrastructure in individual patches
> before adding any of the actual reflink implementation....
Pardon all the dust, I figured that it'd be better to get all the patches
out for earlier review than to make everyone wait until I could get a
reasonable refactoring done once.
> Also, the flag needs to go into the di_flags2 field, as the last
> flag in the di_flags field is reserved for a "more flags" flag if we
> ever need to add more flags to a v2 inode in a v4 filesystem...
Ok.
> > +/*
> > + * xfs_is_reflink_inode() -- Decide if an inode needs to be checked for CoW.
> > + *
> > + * @ip: XFS inode
> > + */
> > +bool
> > +xfs_is_reflink_inode(
> > + struct xfs_inode *ip) /* XFS inode */
> > +{
> > + struct xfs_mount *mp = ip->i_mount;
> > +
> > + if (!xfs_sb_version_hasreflink(&mp->m_sb))
> > + return false;
> > + if (!(ip->i_d.di_flags & XFS_DIFLAG_REFLINK))
> > + return false;
> > +
> > + ASSERT(!XFS_IS_REALTIME_INODE(ip));
> > + return true;
>
> I would have thought you only need to check the inode flag here
> because the only time it will be set is on a reflink enabled
> filesystem. i.e. that flag being set implies we've already done
> all the "reflink is supported in this filesystem and it's not a
> realtime file" checks when setting the flag.
Sure. The reason for so many ASSERTs everywhere is to help me check my
own sanity while cobbling together the first version. I imagine I could
eliminate a lot of them, but since they all compile out on !XFS_DEBUG &&
!XFS_WARN, I didn't think it was a serious problem. :)
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
