xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [RFC PATCH 2/4] xfs: add xfs_create_tmpfile() for O_TMPFILE support
From: Zhi Yong Wu <zwu.kernel@xxxxxxxxx>
Date: Thu, 28 Nov 2013 10:41:24 +0800
Cc: xfstests <xfs@xxxxxxxxxxx>, Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=wYeCS3vBOEAY6Lv44QKa9QcpeGOL367GObkNzx/1F7A=; b=Xix49Qr+MyJ7e81F8HpbeW/VMPbAYidhhf5Uef1uCTxux3eNbfX6QzWBNQ1NpdCb8M fDXp4sBHVv6FpM1AjM/OTH+4D9DgyG0mavg0YgrqBa1vil2gw6fzL9EUveLpY6Sa0wf/ 3t8stK2y7wV/1FtFA4RrH0Dpo2gl6RhhJcRYgqiLc/A7Eko1kjKRRV0AMMudz16JUUwX OU3bd00vvrkm11IxovwAC9ALt5XbF6aFlxItHiANmv9oODVMQTxd6l+KQlLMHDfNEnb4 Kmbbsy+xelRqM92eJZzI/VTQG1LmLnTBUe80+lpL1N6+fzMsYYnxRhPXVf/1NxvnH5pC 8u/Q==
In-reply-to: <20131125213601.GH8803@dastard>
References: <1385379154-3802-1-git-send-email-zwu.kernel@xxxxxxxxx> <1385379154-3802-3-git-send-email-zwu.kernel@xxxxxxxxx> <20131125213601.GH8803@dastard>
On Tue, Nov 26, 2013 at 5:36 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> 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(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index 1c23bdf..e1832de 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -1337,6 +1337,135 @@ xfs_create(
>>  }
>>
>>  int
>> +xfs_create_tmpfile(
>> +     xfs_mount_t             *mp,
>> +     umode_t                 mode,
>> +     dev_t                   rdev,
>> +     xfs_inode_t             **ipp)
>> +{
>> +     struct xfs_inode        *ip = NULL;
>> +     struct xfs_trans        *tp = NULL;
>> +     int                     error;
>> +     uint                    cancel_flags = XFS_TRANS_RELEASE_LOG_RES;
>> +     struct xfs_dquot        *udqp = NULL;
>> +     struct xfs_dquot        *gdqp = NULL;
>> +     struct xfs_dquot        *pdqp = NULL;
>> +     struct xfs_trans_res    *tres;
>> +     uint                    resblks;
>> +
>> +     if (XFS_FORCED_SHUTDOWN(mp))
>> +             return XFS_ERROR(EIO);
>> +
>> +     /*
>> +      * Make sure that we have allocated dquot(s) on disk.
>> +      */
>> +     error = xfs_qm_vop_dqalloc(NULL, mp, xfs_kuid_to_uid(current_fsuid()),
>> +                                     xfs_kgid_to_gid(current_fsgid()),
>> +                                     XFS_PROJID_DEFAULT,
>> +                                     XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
>> +                                     &udqp, &gdqp, &pdqp);
>> +     if (error)
>> +             return error;
>
> Is XFS_PROJID_DEFAULT correct here? If we are getting a parent inode
> from ->tmpfile, then this should be handled the same way as for
> xfs_create.
ok, will fix it.
>
>> +
>> +     resblks = XFS_CREATE_TMPFILE_SPACE_RES(mp);
>> +     tp = xfs_trans_alloc(mp, XFS_TRANS_CREATE_TMPFILE);
>> +
>> +     /*
>> +      * 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.
>> +      */
>> +     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);
>> +     }
>> +     if (error) {
>> +             cancel_flags = 0;
>> +             goto out_trans_cancel;
>> +     }
>
> I don't think this is necessary here. The ENOSPC flushing in
> xfs_create() is done to ensure we have space for directory block
> creation, not so much for inode allocation. Hence it doesn't make a
> lot of sense to have this here....
Good explaination, will fix it, thanks.
>
>> +     /*
>> +      * Reserve disk quota and the inode.
>> +      */
>> +     error = xfs_trans_reserve_quota(tp, mp, udqp, gdqp,
>> +                                             pdqp, resblks, 1, 0);
>> +     if (error)
>> +             goto out_trans_cancel;
>> +
>> +     /*
>> +      * A newly created regular or special file just has one directory
>> +      * entry pointing to them, but a directory also the "." entry
>> +      * pointing to itself.
>> +      */
>> +     error = xfs_dir_ialloc(&tp, NULL, mode, 1, rdev,
>> +                             XFS_PROJID_DEFAULT, resblks > 0,
>> +                             &ip, NULL);
>> +     if (error) {
>> +             if (error == ENOSPC)
>> +                     goto out_trans_cancel;
>> +             goto out_trans_abort;
>> +     }
>> +
>> +     /*
>> +      * If this is a synchronous mount, make sure that the
>> +      * create transaction goes to disk before returning to
>> +      * the user.
>> +      */
>> +     if (mp->m_flags & (XFS_MOUNT_WSYNC|XFS_MOUNT_DIRSYNC))
>> +             xfs_trans_set_sync(tp);
>
> I'm not sure that XFS_MOUNT_DIRSYNC shoul dbe checked here, as there
> is no directory operations to synchronise at all...
ok, will fix it.
>
>> +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);
>> +}
>
> What exactly is being modified here? All of the other reservations
> have comments explaining what each of the individual components of
> the reservation are. Yes, I can see that there's an inode, a
> filesystem block and 2 sectors being reserved, but what are they?
Will add some comments in next version.
>
> It's also double counting the XFS_DQUOT_LOGRES(), as they are
> accounted for in xfs_calc_[i]create_reservation().
I don't understand why you said it double counts XFS_DQUOT_LOGRES()? i
only see it is counted once. Can you say more details?

>
> Also, the directory create reservations are broken up into two
> components - the inode allocation and the common directory name
> creation component, and so the tmpfile reservation should probably
> be broken up the same way. i.e this:
>
>> +
>> +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);
>> +}
>
> Could simply be:
>
> STATIC uint
> xfs_calc_create_tmpfile_reservation(
>         struct xfs_mount        *mp)
> {
>         uint    res;
>
>         if (xfs_sb_version_hascrc(&mp->m_sb))
>                 res = xfs_calc_icreate_resv_alloc(mp);
>         else
>                 res = xfs_calc_create_resv_alloc(mp);
>         return res + xfs_calc_iunlink_add_resv(mp);
> }
Good suggestion, will apply this.
>
>
>> diff --git a/fs/xfs/xfs_trans_space.h b/fs/xfs/xfs_trans_space.h
>> index 7d2c920..16fba89 100644
>> --- a/fs/xfs/xfs_trans_space.h
>> +++ b/fs/xfs/xfs_trans_space.h
>> @@ -61,6 +61,8 @@
>>       (XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK) + XFS_B_TO_FSB(mp, v))
>>  #define      XFS_CREATE_SPACE_RES(mp,nl)     \
>>       (XFS_IALLOC_SPACE_RES(mp) + XFS_DIRENTER_SPACE_RES(mp,nl))
>> +#define      XFS_CREATE_TMPFILE_SPACE_RES(mp)        \
>> +     XFS_IALLOC_SPACE_RES(mp)
>
> I wouldn't bother with this macro - just use XFS_IALLOC_SPACE_RES()
> directly.
ok, will remove this macro.

>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx



-- 
Regards,

Zhi Yong Wu

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