On Wed, Apr 20, 2011 at 12:07 PM, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote:
> On Tue, 19 Apr 2011 08:21:09 -0500
> shirishpargaonkar@xxxxxxxxx wrote:
>
>> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>>
>>
>> rb tree search and insertion routines.
>>
>> A SID which needs to be mapped, is looked up in one of the rb trees
>> depending whether SID is either owner or group SID.
>> If found in the tree, a (mapped) id from that node is assigned to
>> uid or gid as appropriate. If unmapped, an upcall is attempted to
>> map the SID to an id. If upcall is successful, node is marked as
>> mapped. If upcall fails, node stays marked as unmapped and a mapping
>> is attempted again only after an arbitrary time period has passed.
>>
>> To map a SID, which can be either a Owner SID or a Group SID, key
>> description starts with the string "os" or "gs" followed by SID converted
>> to a string. Without "os" or "gs", cifs.upcall does not know whether
>> SID needs to be mapped to either an uid or a gid.
>>
>> Nodes in rb tree have fields to prevent multiple upcalls for
>> a SID. Adding and removing nodes is done within global locks.
>> Shrinker routine prunes a node if it has expired but does not prne
>> an expired node if its refcount is not zero (i.e. sid of that node
>> is being mapped via an upcall).
>> Every time an existing node is accessed, its timestamp is updated
>> to prevent it from getting erased.
>>
>> For now, cifs.upcall is only used to map a SID to an id (uid or gid) but
>> it would be used to obtain an SID for an id.
>>
>>
>> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
>> ---
>> fs/cifs/cifsacl.c | 332 +++++++++++++++++++++++++++++++++++++++++++++++++----
>> fs/cifs/cifsacl.h | 25 ++++
>> 2 files changed, 333 insertions(+), 24 deletions(-)
>>
>> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
>> index cad8d9b..bbc29b8 100644
>> --- a/fs/cifs/cifsacl.c
>> +++ b/fs/cifs/cifsacl.c
>
> [...]
>
>> + /*
>> + * Shrinker could be accessing this entry but it will not
>> + * get erased at this point in time and to make it stay further,
>> + * pretty soon we will take a reference on it only to
>> + * release the reference only after upcall returns (or we are
>> + * woken up), however longer that upcall may take to return.
>> + */
>> + if (!test_and_set_bit(SID_ID_PENDING, &psidid->state)) {
>> + /* paidid and its fields are being accessed sans spinlock */
>> + ++psidid->refcount;
> ^^^
> Huh? You're incrementing and decrementing the
> refcount without any locking at all? What prevents the
> shrinker from finding the entry just before you bump
> the refcount and wiping it out anyway?
>
Becaue we just updated the timestamp so shrinker will not evict the
entry at least for the duration of SID_MAP_EXPIRE.
The refcount is to account for an lenghty upcall.
>> +struct cifs_sid_id {
>> + unsigned int refcount;
>> + unsigned long id;
>> + unsigned long time;
>> + unsigned long retry;
>
> Why, exactly do we need "time" and "retry"? Wouldn't one timestamp
> field do here?
>
One timestamp is to make an entry eligible for eviction and the other
one is to attempt mapping again if previous attempt to map a sid had
failed.
>> + unsigned long state;
>> + char *sidstr;
>> + struct rb_node rbnode;
>> + struct cifs_sid sid;
>> +};
>> +
>
> --
> Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
>
--
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
