From edee0234fef8d9e2147a3a10914bbef69a912d69 Mon Sep 17 00:00:00 2001 From: yury Date: Thu, 17 Jun 2004 15:23:22 +0000 Subject: [PATCH] - added error handling in various places of lmv code. - fixed error code leakage in filter_precreate() what led to endless loop in the case of -ENOSPC due to real lack of space or inability to allocate new inode. - added various error messages in error code pathes in lmv code. - small changes in existent error messages. --- lustre/lmv/lmv_obd.c | 63 ++++++++++++--------- lustre/mds/handler.c | 2 +- lustre/mds/mds_lmv.c | 137 +++++++++++++++++++++++++++++----------------- lustre/mds/mds_reint.c | 35 +++++++----- lustre/obdfilter/filter.c | 10 +++- lustre/tests/sanity.sh | 3 +- 6 files changed, 156 insertions(+), 94 deletions(-) diff --git a/lustre/lmv/lmv_obd.c b/lustre/lmv/lmv_obd.c index 68d36f9..b9c4825 100644 --- a/lustre/lmv/lmv_obd.c +++ b/lustre/lmv/lmv_obd.c @@ -314,7 +314,7 @@ int lmv_check_connect(struct obd_device *obd) { rc2 = obd_disconnect(tgts->ltd_exp, 0); if (rc2) CERROR("error: LMV target %s disconnect on MDT idx %d: " - "rc = %d\n", uuid.uuid, i, rc2); + "error %d\n", uuid.uuid, i, rc2); } class_disconnect(exp, 0); RETURN (rc); @@ -372,7 +372,7 @@ static int lmv_disconnect(struct obd_export *exp, int flags) } out_local: - /* This is the case when no real connection is established by + /* this is the case when no real connection is established by * lmv_check_connect(). */ if (!lmv->connected) class_export_put(exp); @@ -617,10 +617,8 @@ static int lmv_valid_attrs(struct obd_export *exp, struct ll_fid *fid) rc = lmv_check_connect(obd); if (rc) RETURN(rc); - CDEBUG(D_OTHER, "validate %lu/%lu/%lu\n", - (unsigned long) fid->mds, - (unsigned long) fid->id, - (unsigned long) fid->generation); + CDEBUG(D_OTHER, "validate %lu/%lu/%lu\n", (unsigned long) fid->mds, + (unsigned long) fid->id, (unsigned long) fid->generation); LASSERT(fid->mds < lmv->desc.ld_tgt_count); rc = md_valid_attrs(lmv->tgts[fid->mds].ltd_exp, fid); RETURN(rc); @@ -662,13 +660,13 @@ int lmv_get_mea_and_update_object(struct obd_export *exp, struct ll_fid *fid) rc = md_getattr(lmv->tgts[fid->mds].ltd_exp, fid, valid, mealen, &req); if (rc) { - CERROR("md_getattr() failed, rc = %d\n", rc); + CERROR("md_getattr() failed, error %d\n", rc); GOTO(cleanup, rc); } rc = mdc_req2lustre_md(exp, req, 0, NULL, &md); if (rc) { - CERROR("mdc_req2lustre_md() failed, rc = %d\n", rc); + CERROR("mdc_req2lustre_md() failed, error %d\n", rc); GOTO(cleanup, rc); } @@ -935,7 +933,7 @@ int lmv_link(struct obd_export *exp, struct mdc_op_data *data, obj = lmv_grab_obj(obd, &data->fid1, 0); if (obj) { rc = raw_name2idx(obj->objcount, data->name, - data->namelen); + data->namelen); data->fid1 = obj->objs[rc].fid; lmv_put_obj(obj); } @@ -1249,6 +1247,7 @@ int lmv_unlink(struct obd_export *exp, struct mdc_op_data *data, struct lmv_obd *lmv = &obd->u.lmv; int rc, i = 0; ENTRY; + rc = lmv_check_connect(obd); if (rc) RETURN(rc); @@ -1318,9 +1317,14 @@ int lmv_init_ea_size(struct obd_export *exp, int easize, int cookiesize) if (lmv->connected == 0) RETURN(0); - /* FIXME: error handling? */ - for (i = 0; i < lmv->desc.ld_tgt_count; i++) + for (i = 0; i < lmv->desc.ld_tgt_count; i++) { rc = obd_init_ea_size(lmv->tgts[i].ltd_exp, easize, cookiesize); + if (rc) { + CERROR("obd_init_ea_size() failed on MDT target %d, " + "error %d.\n", i, rc); + break; + } + } RETURN(rc); } @@ -1342,7 +1346,6 @@ int lmv_obd_create_single(struct obd_export *exp, struct obdo *oa, LASSERT(oa->o_mds < lmv->desc.ld_tgt_count); rc = obd_create(lmv->tgts[oa->o_mds].ltd_exp, oa, &obj_mdp, oti); - LASSERT(rc == 0); RETURN(rc); } @@ -1373,19 +1376,26 @@ int lmv_obd_create(struct obd_export *exp, struct obdo *oa, if (*ea == NULL) { rc = obd_alloc_diskmd(exp, (struct lov_mds_md **)ea); - LASSERT(*ea != NULL); + if (rc < 0) { + CERROR("obd_alloc_diskmd() failed, error %d\n", + rc); + RETURN(rc); + } + + if (*ea == NULL) + RETURN(-EINVAL); } - mea = (struct mea *)*ea; + rc = 0; mfid.id = oa->o_id; mfid.generation = oa->o_generation; - rc = 0; + + mea = (struct mea *)*ea; if (!mea->mea_count || mea->mea_count > lmv->desc.ld_tgt_count) mea->mea_count = lmv->desc.ld_tgt_count; mea->mea_master = -1; - /* FIXME: error handling? */ for (i = 0, c = 0; c < mea->mea_count && i < lmv->desc.ld_tgt_count; i++) { struct lov_stripe_md obj_md; @@ -1401,17 +1411,20 @@ int lmv_obd_create(struct obd_export *exp, struct obdo *oa, continue; } - /* "Master" MDS should always be part of stripped dir, so - scan for it */ + /* "master" MDS should always be part of stripped dir, so scan + for it. */ if (mea->mea_master == -1 && c == mea->mea_count - 1) continue; oa->o_valid = OBD_MD_FLGENER | OBD_MD_FLTYPE | OBD_MD_FLMODE - | OBD_MD_FLUID | OBD_MD_FLGID | OBD_MD_FLID; + | OBD_MD_FLUID | OBD_MD_FLGID | OBD_MD_FLID; rc = obd_create(lmv->tgts[c].ltd_exp, oa, &obj_mdp, oti); - /* FIXME: error handling here */ - LASSERT(rc == 0); + if (rc) { + CERROR("obd_create() failed on MDT target %d, " + "error %d\n", c, rc); + RETURN(rc); + } mea->mea_fids[c].id = oa->o_id; mea->mea_fids[c].generation = oa->o_generation; @@ -1525,9 +1538,9 @@ int lmv_packmd(struct obd_export *exp, struct lov_mds_md **lmmp, RETURN(0); } - if (!*lmmp) { + if (*lmmp == NULL) { OBD_ALLOC(*lmmp, mea_size); - if (!*lmmp) + if (*lmmp == NULL) RETURN(-ENOMEM); } @@ -1562,8 +1575,8 @@ int lmv_unpackmd(struct obd_export *exp, struct lov_stripe_md **mem_tgt, LASSERT(mea_size == mdsize); OBD_ALLOC(*tmea, mea_size); - /* FIXME: error handling here */ - LASSERT(*tmea != NULL); + if (*tmea == NULL) + RETURN(-ENOMEM); if (!disk_src) RETURN(mea_size); diff --git a/lustre/mds/handler.c b/lustre/mds/handler.c index 46f0ef3..6744184 100644 --- a/lustre/mds/handler.c +++ b/lustre/mds/handler.c @@ -815,7 +815,7 @@ int mds_check_mds_num(struct obd_device *obd, struct inode* inode, if (rc) RETURN(rc); if (mea != NULL) { - /* dir is already splitted, check is requested filename + /* dir is already splitted, check if requested filename * should live at this MDS or at another one */ int i; i = mea_name2idx(mea, name, namelen - 1); diff --git a/lustre/mds/mds_lmv.c b/lustre/mds/mds_lmv.c index 92975d8..e50aef5 100644 --- a/lustre/mds/mds_lmv.c +++ b/lustre/mds/mds_lmv.c @@ -47,9 +47,8 @@ int mds_lmv_connect(struct obd_device *obd, char * lmv_name) { struct mds_obd *mds = &obd->u.mds; - struct lustre_handle conn = {0,}; - int valsize, mdsize; - int rc; + struct lustre_handle conn = {0}; + int rc, valsize, value; ENTRY; if (IS_ERR(mds->mds_lmv_obd)) @@ -79,37 +78,38 @@ int mds_lmv_connect(struct obd_device *obd, char * lmv_name) rc = obd_register_observer(mds->mds_lmv_obd, obd); if (rc) { - CERROR("MDS cannot register as observer of LMV %s (%d)\n", - lmv_name, rc); + CERROR("MDS cannot register as observer of LMV %s, " + "rc = %d\n", lmv_name, rc); GOTO(err_discon, rc); } /* retrieve size of EA */ rc = obd_get_info(mds->mds_lmv_exp, strlen("mdsize"), "mdsize", - &valsize, &mdsize); + &valsize, &value); if (rc) GOTO(err_reg, rc); - if (mdsize > mds->mds_max_mdsize) - mds->mds_max_mdsize = mdsize; + + if (value > mds->mds_max_mdsize) + mds->mds_max_mdsize = value; /* find our number in LMV cluster */ rc = obd_get_info(mds->mds_lmv_exp, strlen("mdsnum"), "mdsnum", - &valsize, &mdsize); + &valsize, &value); if (rc) GOTO(err_reg, rc); - mds->mds_num = mdsize; + + mds->mds_num = value; rc = obd_set_info(mds->mds_lmv_exp, strlen("inter_mds"), - "inter_mds", 0, NULL); + "inter_mds", 0, NULL); if (rc) GOTO(err_reg, rc); + RETURN(0); err_reg: - RETURN(rc); - + obd_register_observer(mds->mds_lmv_obd, NULL); err_discon: - /* FIXME: cleanups here! */ obd_disconnect(mds->mds_lmv_exp, 0); mds->mds_lmv_exp = NULL; mds->mds_lmv_obd = ERR_PTR(rc); @@ -119,11 +119,14 @@ err_discon: int mds_lmv_postsetup(struct obd_device *obd) { struct mds_obd *mds = &obd->u.mds; + int rc = 0; ENTRY; + if (mds->mds_lmv_exp) - obd_init_ea_size(mds->mds_lmv_exp, mds->mds_max_mdsize, - mds->mds_max_cookiesize); - RETURN(0); + rc = obd_init_ea_size(mds->mds_lmv_exp, mds->mds_max_mdsize, + mds->mds_max_cookiesize); + + RETURN(rc); } int mds_lmv_disconnect(struct obd_device *obd, int flags) @@ -133,15 +136,15 @@ int mds_lmv_disconnect(struct obd_device *obd, int flags) ENTRY; if (!IS_ERR(mds->mds_lmv_obd) && mds->mds_lmv_exp != NULL) { - obd_register_observer(mds->mds_lmv_obd, NULL); + /* if obd_disconnect fails (probably because the export was + * disconnected by class_disconnect_exports) then we just need + * to drop our ref. */ rc = obd_disconnect(mds->mds_lmv_exp, flags); - /* if obd_disconnect fails (probably because the - * export was disconnected by class_disconnect_exports) - * then we just need to drop our ref. */ - if (rc != 0) + if (rc) class_export_put(mds->mds_lmv_exp); + mds->mds_lmv_exp = NULL; mds->mds_lmv_obd = NULL; } @@ -149,9 +152,8 @@ int mds_lmv_disconnect(struct obd_device *obd, int flags) RETURN(rc); } - int mds_get_lmv_attr(struct obd_device *obd, struct inode *inode, - struct mea **mea, int *mea_size) + struct mea **mea, int *mea_size) { struct mds_obd *mds = &obd->u.mds; int rc; @@ -163,18 +165,17 @@ int mds_get_lmv_attr(struct obd_device *obd, struct inode *inode, /* first calculate mea size */ *mea_size = obd_alloc_diskmd(mds->mds_lmv_exp, (struct lov_mds_md **)mea); - /* FIXME: error handling here */ - LASSERT(*mea != NULL); + if (*mea_size < 0 || *mea == NULL) + return *mea_size < 0 ? *mea_size : -EINVAL; down(&inode->i_sem); rc = fsfilt_get_md(obd, inode, *mea, *mea_size); up(&inode->i_sem); - /* FIXME: error handling here */ + if (rc <= 0) { OBD_FREE(*mea, *mea_size); *mea = NULL; - } - if (rc > 0) + } else rc = 0; RETURN(rc); @@ -518,10 +519,10 @@ int mds_splitting_expected(struct obd_device *obd, struct dentry *dentry) } /* - * must not be called on already splitted directories + * must not be called on already splitted directories. */ -int mds_try_to_split_dir(struct obd_device *obd, - struct dentry *dentry, struct mea **mea, int nstripes) +int mds_try_to_split_dir(struct obd_device *obd, struct dentry *dentry, + struct mea **mea, int nstripes) { struct inode *dir = dentry->d_inode; struct mds_obd *mds = &obd->u.mds; @@ -533,7 +534,8 @@ int mds_try_to_split_dir(struct obd_device *obd, /* TODO: optimization possible - we already may have mea here */ if (mds_splitting_expected(obd, dentry) != MDS_EXPECT_SPLIT) - return 0; + RETURN(0); + LASSERT(mea == NULL || *mea == NULL); CDEBUG(D_OTHER, "%s: split directory %u/%lu/%lu\n", @@ -548,52 +550,87 @@ int mds_try_to_split_dir(struct obd_device *obd, * necessary amount of stripes, but on the other hand with this * approach of allocating maximal possible amount of MDS slots, * it would be easier to split the dir over more MDSes */ - rc = obd_alloc_diskmd(mds->mds_lmv_exp, (void *) mea); - if (!(*mea)) - RETURN(-ENOMEM); + rc = obd_alloc_diskmd(mds->mds_lmv_exp, (void *)mea); + if (rc < 0) { + CERROR("obd_alloc_diskmd() failed, error %d.\n", rc); + RETURN(rc); + } + if (*mea == NULL) + RETURN(-EINVAL); + (*mea)->mea_count = nstripes; /* 1) create directory objects on slave MDS'es */ /* FIXME: should this be OBD method? */ oa = obdo_alloc(); - /* FIXME: error handling here */ - LASSERT(oa != NULL); + if (!oa) + RETURN(-ENOMEM); + oa->o_id = dir->i_ino; oa->o_generation = dir->i_generation; - obdo_from_inode(oa, dir, OBD_MD_FLTYPE | OBD_MD_FLATIME | + + obdo_from_inode(oa, dir, OBD_MD_FLTYPE | OBD_MD_FLATIME | OBD_MD_FLMTIME | OBD_MD_FLCTIME | OBD_MD_FLUID | OBD_MD_FLGID); + oa->o_gr = FILTER_GROUP_FIRST_MDS + mds->mds_num; oa->o_valid |= OBD_MD_FLID | OBD_MD_FLFLAGS | OBD_MD_FLGROUP; oa->o_mode = dir->i_mode; + CDEBUG(D_OTHER, "%s: create subdirs with mode %o, uid %u, gid %u\n", obd->obd_name, dir->i_mode, dir->i_uid, dir->i_gid); rc = obd_create(mds->mds_lmv_exp, oa, - (struct lov_stripe_md **) mea, NULL); - /* FIXME: error handling here */ - LASSERT(rc == 0); - CDEBUG(D_OTHER, "%d dirobjects created\n", - (int) (*mea)->mea_count); + (struct lov_stripe_md **)mea, NULL); + if (rc) + GOTO(err_oa, rc); + + CDEBUG(D_OTHER, "%d dirobjects created\n", (int)(*mea)->mea_count); /* 2) update dir attribute */ down(&dir->i_sem); + handle = fsfilt_start(obd, dir, FSFILT_OP_SETATTR, NULL); - LASSERT(!IS_ERR(handle)); + if (IS_ERR(handle)) { + up(&dir->i_sem); + CERROR("fsfilt_start() failed, error %d.\n", + PTR_ERR(handle)); + GOTO(err_oa, rc = PTR_ERR(handle)); + } + rc = fsfilt_set_md(obd, dir, handle, *mea, mea_size); - LASSERT(rc == 0); - fsfilt_commit(obd, mds->mds_sb, dir, handle, 0); - LASSERT(rc == 0); + if (rc) { + up(&dir->i_sem); + CERROR("fsfilt_set_md() failed, error %d.\n", rc); + GOTO(err_oa, rc); + } + + rc = fsfilt_commit(obd, mds->mds_sb, dir, handle, 0); + if (rc) { + up(&dir->i_sem); + CERROR("fsfilt_commit() failed, error %d.\n", rc); + GOTO(err_oa, rc); + } + up(&dir->i_sem); obdo_free(oa); /* 3) read through the dir and distribute it over objects */ - scan_and_distribute(obd, dentry, *mea); + rc = scan_and_distribute(obd, dentry, *mea); + if (rc) { + CERROR("scan_and_distribute() failed, error %d.\n", + rc); + RETURN(rc); + } if (mea == &tmea) obd_free_diskmd(mds->mds_lmv_exp, - (struct lov_mds_md **) mea); + (struct lov_mds_md **)mea); RETURN(1); + +err_oa: + obdo_free(oa); + RETURN(rc); } static int filter_start_page_write(struct inode *inode, diff --git a/lustre/mds/mds_reint.c b/lustre/mds/mds_reint.c index 9e9ab01..3135dde 100644 --- a/lustre/mds/mds_reint.c +++ b/lustre/mds/mds_reint.c @@ -626,7 +626,7 @@ static int mds_reint_create(struct mds_update_record *rec, int offset, /* dir got splitted */ GOTO(cleanup, rc = -ERESTART); } else { - /* error happened during spitting */ + /* error happened during spitting. */ GOTO(cleanup, rc); } } @@ -656,13 +656,12 @@ static int mds_reint_create(struct mds_update_record *rec, int offset, /* as Peter asked, mkdir() should distribute new directories * over the whole cluster in order to distribute namespace - * processing load. first, we calculate which MDS to use to - * put new directory's inode in */ + * processing load. first, we calculate which MDS to use to put + * new directory's inode in. */ i = mds_choose_mdsnum(obd, rec->ur_name, rec->ur_namelen - 1, rec->ur_flags); if (i == mds->mds_num) { /* inode will be created locally */ - handle = fsfilt_start(obd, dir, FSFILT_OP_MKDIR, NULL); if (IS_ERR(handle)) GOTO(cleanup, rc = PTR_ERR(handle)); @@ -673,9 +672,17 @@ static int mds_reint_create(struct mds_update_record *rec, int offset, nstripes = *(u16 *)rec->ur_eadata; if (rc == 0 && nstripes) { - /* FIXME: error handling here */ - mds_try_to_split_dir(obd, dchild, - NULL, nstripes); + if ((rc = mds_try_to_split_dir(obd, dchild, + NULL, nstripes))) { + if (rc > 0) { + /* dir got splitted */ + rc = 0; + } else { + /* an error occured during + * splitting. */ + GOTO(cleanup, rc); + } + } } } else if (!DENTRY_VALID(dchild)) { /* inode will be created on another MDS */ @@ -834,11 +841,10 @@ cleanup: err = mds_finish_transno(mds, dir, handle, req, rc, 0); if (rc && created) { - /* Destroy the file we just created. This should not need - * extra journal credits, as we have already modified all of - * the blocks needed in order to create the file in the first - * place. - */ + /* Destroy the file we just created. This should not need extra + * journal credits, as we have already modified all of the + * blocks needed in order to create the file in the first + * place. */ switch (type) { case S_IFDIR: err = vfs_rmdir(dir, dchild); @@ -2630,8 +2636,7 @@ cleanup: } static int mds_reint_rename(struct mds_update_record *rec, int offset, - struct ptlrpc_request *req, - struct lustre_handle *lockh) + struct ptlrpc_request *req, struct lustre_handle *lockh) { struct obd_device *obd = req->rq_export->exp_obd; struct dentry *de_srcdir = NULL; @@ -2682,7 +2687,7 @@ static int mds_reint_rename(struct mds_update_record *rec, int offset, if (de_old->d_inode->i_ino == de_srcdir->d_inode->i_ino || de_old->d_inode->i_ino == de_tgtdir->d_inode->i_ino) GOTO(cleanup, rc = -EINVAL); - + /* sanity check for dest inode */ if (de_new->d_inode && (de_new->d_inode->i_ino == de_srcdir->d_inode->i_ino || diff --git a/lustre/obdfilter/filter.c b/lustre/obdfilter/filter.c index 2d4360b..689b54c 100644 --- a/lustre/obdfilter/filter.c +++ b/lustre/obdfilter/filter.c @@ -2046,13 +2046,14 @@ static int filter_should_precreate(struct obd_export *exp, struct obdo *oa, static int filter_precreate_rec(struct obd_device *obd, struct dentry *dentry, int *number, struct obdo *oa) { - int rc = 0; + int rc; ENTRY; rc = fsfilt_precreate_rec(obd, dentry, number, oa); RETURN(rc); } + /* We rely on the fact that only one thread will be creating files in a given * group at a time, which is why we don't need an atomic filter_get_new_id. * Even if we had that atomic function, the following race would exist: @@ -2183,9 +2184,13 @@ static int filter_precreate(struct obd_device *obd, struct obdo *oa, if (rc) break; } + *num = i; - rc = filter_precreate_rec(obd, dparent, num, oa); + /* check if we have an error after ll_vfs_create(). It is possible that + * there will be say -ENOSPC and we will leak it. */ + if (rc == 0) + rc = filter_precreate_rec(obd, dparent, num, oa); up(&filter->fo_create_lock); @@ -2194,6 +2199,7 @@ static int filter_precreate(struct obd_device *obd, struct obdo *oa, CDEBUG(D_HA, "%s: filter_precreate() created %d objects\n", obd->obd_name, i); + RETURN(rc); } diff --git a/lustre/tests/sanity.sh b/lustre/tests/sanity.sh index f380b63..2aea097 100644 --- a/lustre/tests/sanity.sh +++ b/lustre/tests/sanity.sh @@ -8,7 +8,8 @@ set -e ONLY=${ONLY:-"$*"} # bug number for skipped test: 2108 -ALWAYS_EXCEPT=${ALWAYS_EXCEPT:-"24a 24b 24c 24d 24e 24f 24g 24h 24i 24j 24k 24l 24m 24n 58"} +RENAME_TESTS="24a 24b 24c 24d 24e 24f 24g 24h 24i 24j 24k 24l 24m 24n 24o 48a 48d" +ALWAYS_EXCEPT=${ALWAYS_EXCEPT:-"$RENAME_TESTS 58"} # UPDATE THE COMMENT ABOVE WITH BUG NUMBERS WHEN CHANGING ALWAYS_EXCEPT! case `uname -r` in 2.6.*) ALWAYS_EXCEPT="$ALWAYS_EXCEPT 54c 55" # bug 3117 -- 1.8.3.1