On Mon, Mar 28, 2011 at 2:02 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Thu, 24 Mar 2011 12:00:24 -0500
> shirishpargaonkar@xxxxxxxxx wrote:
>
>
>> +static int
>> +sid_to_id(struct cifs_sb_info *cifs_sb, struct cifs_sid *psid,
>> + struct cifs_fattr *fattr, uint sidtype)
>> +{
>> + int rc;
>> + unsigned long cid;
>> + char *strptr;
>> + struct key *idkey;
>> + const struct cred *saved_cred;
>> + struct cifs_sid_id *psidid, *npsidid;
>> + struct rb_root *cidtree;
>> + spinlock_t *cidlock;
>> +
>> + if (sidtype == SIDOWNER) {
>> + cid = cifs_sb->mnt_uid; /* default uid, in case upcall fails */
>> + cidlock = &siduidlock;
>> + cidtree = &uidtree;
>> + } else if (sidtype == SIDGROUP) {
>> + cid = cifs_sb->mnt_gid; /* default gid, in case upcall fails */
>> + cidlock = &sidgidlock;
>> + cidtree = &gidtree;
>> + } else
>> + return -ENOENT;
>> +
>
> That default seems reasonable, I think...
>
>> + spin_lock(cidlock);
>> + psidid = id_rb_search(cidtree, psid);
>> + spin_unlock(cidlock);
>> +
>> + if (!psidid) { /* node does not exist, allocate one & attempt adding */
>> + npsidid = kzalloc(sizeof(struct cifs_sid_id), GFP_KERNEL);
>> + if (!npsidid)
>> + return -ENOMEM;
>> +
>> + spin_lock(cidlock);
>> + psidid = id_rb_search(cidtree, psid);
>> + if (psidid) { /* node happened to get inserted meanwhile */
>> + spin_unlock(cidlock);
>> + kfree(npsidid);
>> + } else {
>> + psidid = npsidid;
>> + id_rb_insert(cidtree, psid, &psidid);
>> + spin_unlock(cidlock);
>> + }
>> + }
>> +
>> + if (!test_and_set_bit(SID_ID_PENDING, &psidid->state)) {
>> + if (!test_bit(SID_ID_MAPPED, &psidid->state)) {
>
> Overall this looks a lot better. The above however is a bit
> questionable. Every time you find an entry in the tree, you then mark
> it PENDING and check to see if it's MAPPED.
>
> These entries are never going to go from being MAPPED to not. Would it
> instead make more sense to check whether it's MAPPED first, and only
> then try to set the PENDING bit?
>
> if (!test_bit(SID_ID_MAPPED, &psidid->state) &&
> !test_and_set_bit(SID_ID_PENDING, &psidid->state)) {
> /* construct the entry */
> } else {
> /* wait on the PENDING bit */
> }
>
> The reason for this would be to minimize the times that you're going to
> set the PENDING bit, and force other threads to serialize behind it.
>
>> + /* node with unmapped SID */
>> + memset(psidid->sidstr, '\0', SIDLEN);
>> + if (sidtype == SIDOWNER)
>> + sprintf(psidid->sidstr, "%s", "os:");
>> + else
>> + sprintf(psidid->sidstr, "%s", "gs:");
>> +
>> + strptr = psidid->sidstr + strlen(psidid->sidstr);
>> + sid_to_str(psid, strptr);
>> +
>> + saved_cred = override_creds(root_cred);
>> + idkey = request_key(&cifs_idmap_key_type,
>> + psidid->sidstr, "");
>> + if (IS_ERR(idkey)) {
>> + psidid->id = cid;
>> + cFYI(1, "%s: Can't map SID to an id", __func__);
>> + } else {
>> + psidid->id =
>> + *(unsigned long *)idkey->payload.value;
>> + psidid->time = jiffies;
>> + set_bit(SID_ID_MAPPED, &psidid->state);
>> + key_put(idkey);
>> + }
>> + revert_creds(saved_cred);
>> + }
>> + clear_bit(SID_ID_PENDING, &psidid->state);
>> + wake_up_bit(&psidid->state, SID_ID_PENDING);
>> + } else {
>> + /*
>> + * either an existing node with SID mapped (in which case
>> + * we would not wait) or a node whose SID is being mapped by
>> + * somebody else (in which case, we would end up waiting).
>> + */
>> + rc = wait_on_bit(&psidid->state, SID_ID_PENDING,
>> + sidid_pending_wait, TASK_INTERRUPTIBLE);
>> + if (rc) {
>> + cFYI(1, "%s: sidid_pending_wait interrupted %d",
>> + __func__, rc);
>> + return rc;
>> + }
>> + }
>> +
>> +
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
Jeff, Thanks.
yes, that is correct. Testing for a mapped id before attempting to
do map it if not, makes sense.
Will respin and resubmit the patch.
Regards,
Shirish
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
