Message ID | 20201207001533.2702719-4-hsiangkao@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | xfs: some xfs_dialloc() cleanup | expand |
On Mon, Dec 07, 2020 at 08:15:30AM +0800, Gao Xiang wrote: > /* > + * Initialise a newly allocated inode and return the in-core inode to the > + * caller locked exclusively. > */ > -static int > -xfs_ialloc( > - xfs_trans_t *tp, > - xfs_inode_t *pip, > - umode_t mode, > - xfs_nlink_t nlink, > - dev_t rdev, > - prid_t prid, > - xfs_buf_t **ialloc_context, > - xfs_inode_t **ipp) > +static struct xfs_inode * > +xfs_dir_ialloc_init( This is boderline bikeshedding, but I would just call this xfs_init_new_inode. > int > xfs_dir_ialloc( > @@ -954,83 +908,59 @@ xfs_dir_ialloc( > xfs_inode_t **ipp) /* pointer to inode; it will be > locked. */ > { > xfs_inode_t *ip; > xfs_buf_t *ialloc_context = NULL; > + xfs_ino_t pino = dp ? dp->i_ino : 0; Maybe spell out parent_inode? pino reminds of some of the weird Windows code that start all variable names for pointers with a "p". > + /* Initialise the newly allocated inode. */ > + ip = xfs_dir_ialloc_init(*tpp, dp, ino, mode, nlink, rdev, prid); > + if (IS_ERR(ip)) > + return PTR_ERR(ip); > + *ipp = ip; > return 0; I wonder if we should just return the inode by reference from xfs_dir_ialloc_init as well, as that nicely fits the calling convention in the caller, i.e. this could become return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid, ipp); Note with the right naming we don't really need the comment either, as the function name should explain everything.
On Mon, Dec 07, 2020 at 02:49:41PM +0100, Christoph Hellwig wrote: > On Mon, Dec 07, 2020 at 08:15:30AM +0800, Gao Xiang wrote: > > /* > > + * Initialise a newly allocated inode and return the in-core inode to the > > + * caller locked exclusively. > > */ > > -static int > > -xfs_ialloc( > > - xfs_trans_t *tp, > > - xfs_inode_t *pip, > > - umode_t mode, > > - xfs_nlink_t nlink, > > - dev_t rdev, > > - prid_t prid, > > - xfs_buf_t **ialloc_context, > > - xfs_inode_t **ipp) > > +static struct xfs_inode * > > +xfs_dir_ialloc_init( > > This is boderline bikeshedding, but I would just call this > xfs_init_new_inode. (See below...) > > > int > > xfs_dir_ialloc( > > @@ -954,83 +908,59 @@ xfs_dir_ialloc( > > xfs_inode_t **ipp) /* pointer to inode; it will be > > locked. */ > > { > > xfs_inode_t *ip; > > xfs_buf_t *ialloc_context = NULL; > > + xfs_ino_t pino = dp ? dp->i_ino : 0; > > Maybe spell out parent_inode? pino reminds of some of the weird Windows > code that start all variable names for pointers with a "p". Ok, yet pino is somewhat common, as I saw it in f2fs and jffs2 before. I know you mean 'Hungarian naming conventions'. If you don't like pino. How about parent_ino? since parent_inode occurs me about "struct inode *" or something like this (a pointer around some inode), rather than an inode number. > > > + /* Initialise the newly allocated inode. */ > > + ip = xfs_dir_ialloc_init(*tpp, dp, ino, mode, nlink, rdev, prid); > > + if (IS_ERR(ip)) > > + return PTR_ERR(ip); > > + *ipp = ip; > > return 0; > > I wonder if we should just return the inode by reference from > xfs_dir_ialloc_init as well, as that nicely fits the calling convention > in the caller, i.e. this could become > > return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid, ipp); > > Note with the right naming we don't really need the comment either, > as the function name should explain everything. Okay, the name was from Dave to unify the prefix (namespace)... I think it'd be better to get Dave's idea about this as well. As of me, I'm fine with either way. Thanks, Gao Xiang >
On Mon, Dec 07, 2020 at 10:19:48PM +0800, Gao Xiang wrote: > > Maybe spell out parent_inode? pino reminds of some of the weird Windows > > code that start all variable names for pointers with a "p". > > Ok, yet pino is somewhat common, as I saw it in f2fs and jffs2 before. > I know you mean 'Hungarian naming conventions'. > > If you don't like pino. How about parent_ino? since parent_inode occurs me > about "struct inode *" or something like this (a pointer around some inode), > rather than an inode number. Yeah, parent_ino is what I mean to suggest anyway, sorry for the typo.
On Mon, Dec 07, 2020 at 02:49:41PM +0100, Christoph Hellwig wrote: > On Mon, Dec 07, 2020 at 08:15:30AM +0800, Gao Xiang wrote: > > /* > > + * Initialise a newly allocated inode and return the in-core inode to the > > + * caller locked exclusively. > > */ > > -static int > > -xfs_ialloc( > > - xfs_trans_t *tp, > > - xfs_inode_t *pip, > > - umode_t mode, > > - xfs_nlink_t nlink, > > - dev_t rdev, > > - prid_t prid, > > - xfs_buf_t **ialloc_context, > > - xfs_inode_t **ipp) > > +static struct xfs_inode * > > +xfs_dir_ialloc_init( > > This is boderline bikeshedding, but I would just call this > xfs_init_new_inode. > > > int > > xfs_dir_ialloc( > > @@ -954,83 +908,59 @@ xfs_dir_ialloc( > > xfs_inode_t **ipp) /* pointer to inode; it will be > > locked. */ > > { > > xfs_inode_t *ip; > > xfs_buf_t *ialloc_context = NULL; > > + xfs_ino_t pino = dp ? dp->i_ino : 0; > > Maybe spell out parent_inode? pino reminds of some of the weird Windows > code that start all variable names for pointers with a "p". > > > + /* Initialise the newly allocated inode. */ > > + ip = xfs_dir_ialloc_init(*tpp, dp, ino, mode, nlink, rdev, prid); > > + if (IS_ERR(ip)) > > + return PTR_ERR(ip); > > + *ipp = ip; > > return 0; > > I wonder if we should just return the inode by reference from > xfs_dir_ialloc_init as well, as that nicely fits the calling convention > in the caller, i.e. this could become > > return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid, ipp); > > Note with the right naming we don't really need the comment either, > as the function name should explain everything. /me notes that some day he will get around to a formal posting of his giant inode allocation cleanup series that will wrap these parameters into a struct so we don't have to pass 8 arguments around, and fix the inode flag inheritance inconsistencies between userspace and kernel... --D
On Mon, Dec 07, 2020 at 10:19:48PM +0800, Gao Xiang wrote: > On Mon, Dec 07, 2020 at 02:49:41PM +0100, Christoph Hellwig wrote: > > On Mon, Dec 07, 2020 at 08:15:30AM +0800, Gao Xiang wrote: > > > /* > > > + * Initialise a newly allocated inode and return the in-core inode to the > > > + * caller locked exclusively. > > > */ > > > -static int > > > -xfs_ialloc( > > > - xfs_trans_t *tp, > > > - xfs_inode_t *pip, > > > - umode_t mode, > > > - xfs_nlink_t nlink, > > > - dev_t rdev, > > > - prid_t prid, > > > - xfs_buf_t **ialloc_context, > > > - xfs_inode_t **ipp) > > > +static struct xfs_inode * > > > +xfs_dir_ialloc_init( > > > > This is boderline bikeshedding, but I would just call this > > xfs_init_new_inode. > > (See below...) > > > > > > int > > > xfs_dir_ialloc( > > > @@ -954,83 +908,59 @@ xfs_dir_ialloc( > > > xfs_inode_t **ipp) /* pointer to inode; it will be > > > locked. */ > > > { > > > xfs_inode_t *ip; > > > xfs_buf_t *ialloc_context = NULL; > > > + xfs_ino_t pino = dp ? dp->i_ino : 0; > > > > Maybe spell out parent_inode? pino reminds of some of the weird Windows > > code that start all variable names for pointers with a "p". > > Ok, yet pino is somewhat common, as I saw it in f2fs and jffs2 before. > I know you mean 'Hungarian naming conventions'. > > If you don't like pino. How about parent_ino? since parent_inode occurs me > about "struct inode *" or something like this (a pointer around some inode), > rather than an inode number. > > > > > > + /* Initialise the newly allocated inode. */ > > > + ip = xfs_dir_ialloc_init(*tpp, dp, ino, mode, nlink, rdev, prid); > > > + if (IS_ERR(ip)) > > > + return PTR_ERR(ip); > > > + *ipp = ip; > > > return 0; > > > > I wonder if we should just return the inode by reference from > > xfs_dir_ialloc_init as well, as that nicely fits the calling convention > > in the caller, i.e. this could become > > > > return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid, ipp); > > > > Note with the right naming we don't really need the comment either, > > as the function name should explain everything. > > Okay, the name was from Dave to unify the prefix (namespace)... I think it'd > be better to get Dave's idea about this as well. As of me, I'm fine with > either way. I'm fine with that. -Dave.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 6672cdffcda5..22843e81bccf 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -761,68 +761,25 @@ xfs_inode_inherit_flags2( } /* - * Allocate an inode on disk and return a copy of its in-core version. - * The in-core inode is locked exclusively. Set mode, nlink, and rdev - * appropriately within the inode. The uid and gid for the inode are - * set according to the contents of the given cred structure. - * - * Use xfs_dialloc() to allocate the on-disk inode. If xfs_dialloc() - * has a free inode available, call xfs_iget() to obtain the in-core - * version of the allocated inode. Finally, fill in the inode and - * log its initial contents. In this case, ialloc_context would be - * set to NULL. - * - * If xfs_dialloc() does not have an available inode, it will replenish - * its supply by doing an allocation. Since we can only do one - * allocation within a transaction without deadlocks, we must commit - * the current transaction before returning the inode itself. - * In this case, therefore, we will set ialloc_context and return. - * The caller should then commit the current transaction, start a new - * transaction, and call xfs_ialloc() again to actually get the inode. - * - * To ensure that some other process does not grab the inode that - * was allocated during the first call to xfs_ialloc(), this routine - * also returns the [locked] bp pointing to the head of the freelist - * as ialloc_context. The caller should hold this buffer across - * the commit and pass it back into this routine on the second call. - * - * If we are allocating quota inodes, we do not have a parent inode - * to attach to or associate with (i.e. pip == NULL) because they - * are not linked into the directory structure - they are attached - * directly to the superblock - and so have no parent. + * Initialise a newly allocated inode and return the in-core inode to the + * caller locked exclusively. */ -static int -xfs_ialloc( - xfs_trans_t *tp, - xfs_inode_t *pip, - umode_t mode, - xfs_nlink_t nlink, - dev_t rdev, - prid_t prid, - xfs_buf_t **ialloc_context, - xfs_inode_t **ipp) +static struct xfs_inode * +xfs_dir_ialloc_init( + struct xfs_trans *tp, + struct xfs_inode *pip, + xfs_ino_t ino, + umode_t mode, + xfs_nlink_t nlink, + dev_t rdev, + prid_t prid) { - struct xfs_mount *mp = tp->t_mountp; - xfs_ino_t ino; - xfs_inode_t *ip; - uint flags; - int error; - struct timespec64 tv; - struct inode *inode; - - /* - * Call the space management code to pick - * the on-disk inode to be allocated. - */ - error = xfs_dialloc(tp, pip ? pip->i_ino : 0, mode, - ialloc_context, &ino); - if (error) - return error; - if (*ialloc_context || ino == NULLFSINO) { - *ipp = NULL; - return 0; - } - ASSERT(*ialloc_context == NULL); + struct xfs_mount *mp = tp->t_mountp; + struct xfs_inode *ip; + unsigned int flags; + int error; + struct timespec64 tv; + struct inode *inode; /* * Protect against obviously corrupt allocation btree records. Later @@ -833,18 +790,16 @@ xfs_ialloc( */ if ((pip && ino == pip->i_ino) || !xfs_verify_dir_ino(mp, ino)) { xfs_alert(mp, "Allocated a known in-use inode 0x%llx!", ino); - return -EFSCORRUPTED; + return ERR_PTR(-EFSCORRUPTED); } /* - * Get the in-core inode with the lock held exclusively. - * This is because we're setting fields here we need - * to prevent others from looking at until we're done. + * Get the in-core inode with the lock held exclusively to prevent + * others from looking at until we're done. */ - error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE, - XFS_ILOCK_EXCL, &ip); + error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, &ip); if (error) - return error; + return ERR_PTR(error); ASSERT(ip != NULL); inode = VFS_I(ip); inode->i_mode = mode; @@ -926,20 +881,19 @@ xfs_ialloc( /* now that we have an i_mode we can setup the inode structure */ xfs_setup_inode(ip); - - *ipp = ip; - return 0; + return ip; } /* - * Allocates a new inode from disk and return a pointer to the - * incore copy. This routine will internally commit the current - * transaction and allocate a new one if the Space Manager needed - * to do an allocation to replenish the inode free-list. - * - * This routine is designed to be called from xfs_create and - * xfs_create_dir. + * Allocates a new inode from disk and return a pointer to the incore copy. This + * routine will internally commit the current transaction and allocate a new one + * if we needed to allocate more on-disk free inodes to perform the requested + * operation. * + * If we are allocating quota inodes, we do not have a parent inode to attach to + * or associate with (i.e. dp == NULL) because they are not linked into the + * directory structure - they are attached directly to the superblock - and so + * have no parent. */ int xfs_dir_ialloc( @@ -954,83 +908,59 @@ xfs_dir_ialloc( xfs_inode_t **ipp) /* pointer to inode; it will be locked. */ { - xfs_trans_t *tp; xfs_inode_t *ip; xfs_buf_t *ialloc_context = NULL; - int code; - - tp = *tpp; - ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES); + xfs_ino_t pino = dp ? dp->i_ino : 0; + xfs_ino_t ino; + int error; - /* - * xfs_ialloc will return a pointer to an incore inode if - * the Space Manager has an available inode on the free - * list. Otherwise, it will do an allocation and replenish - * the freelist. Since we can only do one allocation per - * transaction without deadlocks, we will need to commit the - * current transaction and start a new one. We will then - * need to call xfs_ialloc again to get the inode. - * - * If xfs_ialloc did an allocation to replenish the freelist, - * it returns the bp containing the head of the freelist as - * ialloc_context. We will hold a lock on it across the - * transaction commit so that no other process can steal - * the inode(s) that we've just allocated. - */ - code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context, - &ip); + ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES); + *ipp = NULL; /* - * Return an error if we were unable to allocate a new inode. - * This should only happen if we run out of space on disk or - * encounter a disk error. + * Call the space management code to pick the on-disk inode to be + * allocated and replenish the freelist. Since we can only do one + * allocation per transaction without deadlocks, we will need to + * commit the current transaction and start a new one. + * If xfs_dialloc did an allocation to replenish the freelist, it + * returns the bp containing the head of the freelist as + * ialloc_context. We will hold a lock on it across the transaction + * commit so that no other process can steal the inode(s) that we've + * just allocated. */ - if (code) { - *ipp = NULL; - return code; - } - if (!ialloc_context && !ip) { - *ipp = NULL; - return -ENOSPC; - } + error = xfs_dialloc(*tpp, pino, mode, &ialloc_context, &ino); + if (error) + return error; /* * If the AGI buffer is non-NULL, then we were unable to get an * inode in one operation. We need to commit the current - * transaction and call xfs_ialloc() again. It is guaranteed + * transaction and call xfs_dialloc() again. It is guaranteed * to succeed the second time. */ if (ialloc_context) { - code = xfs_dialloc_roll(&tp, ialloc_context); - if (code) { - *tpp = tp; - *ipp = NULL; - return code; - } - - /* - * Call ialloc again. Since we've locked out all - * other allocations in this allocation group, - * this call should always succeed. - */ - code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, - &ialloc_context, &ip); - + error = xfs_dialloc_roll(tpp, ialloc_context); + if (error) + return error; /* - * If we get an error at this point, return to the caller - * so that the current transaction can be aborted. + * Call dialloc again. Since we've locked out all other + * allocations in this allocation group, this call should + * always succeed. */ - if (code) { - *tpp = tp; - *ipp = NULL; - return code; - } - ASSERT(!ialloc_context && ip); + error = xfs_dialloc(*tpp, pino, mode, &ialloc_context, &ino); + if (error) + return error; + ASSERT(!ialloc_context); } - *ipp = ip; - *tpp = tp; + if (ino == NULLFSINO) + return -ENOSPC; + /* Initialise the newly allocated inode. */ + ip = xfs_dir_ialloc_init(*tpp, dp, ino, mode, nlink, rdev, prid); + if (IS_ERR(ip)) + return PTR_ERR(ip); + *ipp = ip; return 0; }