xfs
[Top] [All Lists]

Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support

To: Zhi Yong Wu <zwu.kernel@xxxxxxxxx>
Subject: Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 25 Nov 2013 05:48:21 -0800
Cc: xfs@xxxxxxxxxxx, Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1385379154-3802-3-git-send-email-zwu.kernel@xxxxxxxxx>
References: <1385379154-3802-1-git-send-email-zwu.kernel@xxxxxxxxx> <1385379154-3802-3-git-send-email-zwu.kernel@xxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Nov 25, 2013 at 07:32:32PM +0800, Zhi Yong Wu wrote:
> From: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
> 
> The function is used to create one O_TMPFILE file.
> 
> http://oss.sgi.com/archives/xfs/2013-08/msg00339.html
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
> ---
>  fs/xfs/xfs_inode.c       |  129 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_inode.h       |    2 +
>  fs/xfs/xfs_shared.h      |    4 +-
>  fs/xfs/xfs_trans_resv.c  |   35 ++++++++++++
>  fs/xfs/xfs_trans_resv.h  |    2 +
>  fs/xfs/xfs_trans_space.h |    2 +
>  6 files changed, 173 insertions(+), 1 deletions(-)

>  int
> +xfs_create_tmpfile(
> +     xfs_mount_t             *mp,
> +     umode_t                 mode,
> +     dev_t                   rdev,
> +     xfs_inode_t             **ipp)

Please use struct xfs_mount and struct xfs_inode for any new code.

> +     /*
> +      * Initially assume that the file does not exist and
> +      * reserve the resources for that case.  If that is not
> +      * the case we'll drop the one we have and get a more
> +      * appropriate transaction later.
> +      */

I can't see how this comment makes any sense.

> +     tres = &M_RES(mp)->tr_create_tmpfile;
> +     error = xfs_trans_reserve(tp, tres, resblks, 0);
> +     if (error == ENOSPC) {
> +             /* flush outstanding delalloc blocks and retry */
> +             xfs_flush_inodes(mp);
> +             error = xfs_trans_reserve(tp, tres, resblks, 0);
> +     }
> +     if (error == ENOSPC) {
> +             /* No space at all so try a "no-allocation" reservation */
> +             resblks = 0;
> +             error = xfs_trans_reserve(tp, tres, 0, 0);
> +     }

Please factor this into a new xfs_trans_reserver_create helper (better
names welcome of course).
similar.

> +     /*
> +      * Reserve disk quota and the inode.
> +      */

I don't think that comment adds a whole lot of value.  (Same for the
other quota comment above).

> +     /*
> +      * A newly created regular or special file just has one directory
> +      * entry pointing to them, but a directory also the "." entry
> +      * pointing to itself.
> +      */

Given that we only create regular files here the comment can be removed.

>  
> +STATIC uint
> +xfs_calc_icreate_tmpfile_reservation(xfs_mount_t *mp)
> +{
> +     return XFS_DQUOT_LOGRES(mp) +
> +             xfs_calc_icreate_resv_alloc(mp) +
> +             xfs_calc_inode_res(mp, 1) +
> +             xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> +}
> +
> +STATIC uint
> +__xfs_calc_create_tmpfile_reservation(
> +     struct xfs_mount        *mp)
> +{
> +     return XFS_DQUOT_LOGRES(mp) +
> +             xfs_calc_create_resv_alloc(mp) +
> +             xfs_calc_inode_res(mp, 1) +
> +             xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> +             xfs_calc_buf_res(2, mp->m_sb.sb_sectsize);
> +}
> +
> +STATIC uint
> +xfs_calc_create_tmpfile_reservation(
> +     struct xfs_mount        *mp)
> +{
> +     if (xfs_sb_version_hascrc(&mp->m_sb))
> +             return xfs_calc_icreate_tmpfile_reservation(mp);
> +     return __xfs_calc_create_tmpfile_reservation(mp);

Shouldn't we name this xfs_calc_create_tmpfile_reservation_v4 and _v5
or no postix and _crc?  Either way the double underscore naming looks
confusing.

<Prev in Thread] Current Thread [Next in Thread>