- 10 Jul, 2014 15 commits
-
-
Trond Myklebust authored
Access to this list is currently serialized by the client_mutex. Add finer grained locking around this list in preparation for its removal. Signed-off-by:
Trond Myklebust <trond.myklebust@primarydata.com> Reviewed-by:
Christoph Hellwig <hch@lst.de> Signed-off-by:
J. Bruce Fields <bfields@redhat.com>
-
Jeff Layton authored
No need to take the lock unless the count goes to 0. Signed-off-by:
Jeff Layton <jlayton@primarydata.com> Reviewed-by:
-
Jeff Layton authored
Bruce says: There's also a preexisting expire_client/laundromat vs break race: - expire_client/laundromat adds a delegation to its local reaplist using the same dl_recall_lru field that a delegation uses to track its position on the recall lru and drops the state lock. - a concurrent break_lease adds the delegation to the lru. - expire/client/laundromat then walks it reaplist and sees the lru head as just another delegation on the list.... Fix this race by checking the dl_time under the state_lock. If we find that it's not 0, then we know that it has already been queued to the LRU list and that we shouldn't queue it again. In the case of destroy_client, we must also ensure that we don't hit similar races by ensuring that we don't move any delegations to the reaplist with a dl_time of 0. Just bump the dl_time by one before we drop the state_lock. We're destroying the delegations anyway, so a 1s difference there won't matter. The fault injection code also requires a bit of surgery here: First, in the case of nfsd_forget_client_delegations, we must prevent the same sort of race vs. the delegation break callback. For that, we just increment the dl_time to ensure that a delegation callback can't race in while we're working on it. We can't do that for nfsd_recall_client_delegations, as we need to have it actually queue the delegation, and that won't happen if we increment the dl_time. The state lock is held over that function, so we don't need to worry about these sorts of races there. There is one other potential bug nfsd_recall_client_delegations though. Entries on the victims list are not dequeued before calling nfsd_break_one_deleg. That's a potential list corruptor, so ensure that we do that there. Reported-by:"J. Bruce Fields" <bfields@fieldses.org> Signed-off-by:
-
Kinglong Mee authored
Commit 8c7424cf (nfsd4: don't try to encode conflicting owner if low on space) forgot free conf->data in nfsd4_encode_lockt and before sign conf->data to NULL in nfsd4_encode_lock_denied. Signed-off-by:
Kinglong Mee <kinglongmee@gmail.com> Signed-off-by:
-
Trond Myklebust authored
lookup_clientid is preferable to find_confirmed_client since it's able to use the cached client in the compound state. Signed-off-by:
-
Trond Myklebust authored
In later patches, we'll be moving the stateowner table into the nfs4_client, and by doing this we ensure that we have a cached nfs4_client pointer. Signed-off-by:
-
Trond Myklebust authored
...and have alloc_init_open_stateowner just use the cstate->clp pointer instead of passing in a clp separately. This allows us to use the cached nfs4_client pointer in the cstate instead of having to look it up again. Signed-off-by:
-
Jeff Layton authored
We want to use the nfsd4_compound_state to cache the nfs4_client in order to optimise away extra lookups of the clid. In the v4.0 case, we use this to ensure that we only have to look up the client at most once per compound for each call into lookup_clientid. For v4.1+ we set the pointer in the cstate during SEQUENCE processing so we should never need to do a search for it. Signed-off-by:
-
Jeff Layton authored
I saw this pop up with some pynfs testing: [ 123.609992] nfsd: non-standard errno: -7 ...and -7 is -E2BIG. I think what happened is that XFS returned -E2BIG due to some xattr operations with the ACL10 pynfs TEST (I guess it has limited xattr size?). Add a better mapping for that error since it's possible that we'll need it. How about we convert it to NFSERR_FBIG? As Bruce points out, they both have "BIG" in the name so it must be good. Also, turn the printk in this function into a WARN() so that we can get a bit more information about situations that don't have proper mappings. Signed-off-by: -
Jeff Layton authored
Commit 2a7420c03e504 (nfsd: Ensure that nfsd_create_setattr commits files to stable storage), added a couple of calls to commit_metadata, but doesn't convert their return codes to __be32 in the appropriate places. Cc: Trond Myklebust <trond.myklebust@primarydata.com> Signed-off-by:
-
Trond Myklebust authored
The cstate already holds information about the session, and hence the client id, so it makes more sense to pass that information rather than the current practice of passing a 'minor version' number. Signed-off-by:
-
Trond Myklebust authored
If the client were to disappear from underneath us while we're holding a session reference, things would be bad. This cleanup helps ensure that it cannot, which will be a possibility when the client_mutex is removed. Signed-off-by:
-
Jeff Layton authored
Now that we know that we won't have several lockowners with the same, owner->data, we can simplify nfsd4_release_lockowner and get rid of the lo_list in the process. Signed-off-by:
-
Trond Myklebust authored
Just like open-owners, lock-owners are associated with a name, a clientid and, in the case of minor version 0, a sequence id. There is no association to a file. Signed-off-by:
-
Jeff Layton authored
A lockowner can have more than one lock stateid. For instance, if a process has more than one file open and has locks on both, then the same lockowner has more than one stateid associated with it. Change it so that this reality is better reflected by the objects that nfsd uses. Signed-off-by:
-
- 08 Jul, 2014 19 commits
-
-
Trond Myklebust authored
In the NFSv4 spec, lock stateids are per-file objects. Lockowners are not. This patch replaces the current list of lock owners in the open stateids with a list of lock stateids. Signed-off-by:
-
Trond Myklebust authored
Minor cleanup that should introduce no behavioral changes. Currently this function just unhashes the stateid and leaves the caller to do the work of the CLOSE processing. Change nfsd4_close_open_stateid so that it handles doing all of the work of closing a stateid. Move the handling of the unhashed stateid into it instead of doing that work in nfsd4_close. This will help isolate some coming changes to stateid handling from nfsd4_close. Signed-off-by:
-
Jeff Layton authored
There's no need to confirm an openowner in v4.1 and above, so we can go ahead and set NFS4_OO_CONFIRMED when we create openowners in those versions. This will also be necessary when we remove the client_mutex, as it'll be possible for two concurrent opens to race in versions >4.0. Signed-off-by:
-
Trond Myklebust authored
Move the slot return, put session etc into a helper in fs/nfsd/nfs4state.c instead of open coding in nfs4svc_encode_compoundres. Signed-off-by:
-
Trond Myklebust authored
Not technically a bugfix, since nothing tries to use the return pointer if this function doesn't return success, but it could be a problem with some coming changes. Signed-off-by:
-
Jeff Layton authored
Currently, the maximum number of connections that nfsd will allow is based on the number of threads spawned. While this is fine for a default, there really isn't a clear relationship between the two. The number of threads corresponds to the number of concurrent requests that we want to allow the server to process at any given time. The connection limit corresponds to the maximum number of clients that we want to allow the server to handle. These are two entirely different quantities. Break the dependency on increasing threads in order to allow for more connections, by adding a new per-net parameter that can be set to a non-zero value. The default is still to base it on the number of threads, so there should be no behavior change for anyone who doesn't use it. Cc: Trond Myklebust <trond.myklebust@primarydata.com> Signed-off-by:
-
Trond Myklebust authored
Since nfsd_create_setattr strips the mode from the struct iattr, it is quite possible that it will optimise away the call to nfsd_setattr altogether. If this is the case, then we never call commit_metadata() on the newly created file. Also ensure that both nfsd_setattr() and nfsd_create_setattr() fail when the call to commit_metadata fails. Signed-off-by:
-
Kinglong Mee authored
Commit db2e747b (vfs: remove mode parameter from vfs_symlink()) have remove mode parameter from vfs_symlink. So that, iattr isn't needed by nfsd_symlink now, just remove it. Signed-off-by:
-
Trond Myklebust authored
Current code depends on the client_mutex to guarantee a single struct nfs4_file per inode in the file_hashtbl and make addition atomic with respect to lookup. Rely instead on the state_Lock, to make it easier to stop taking the client_mutex here later. To prevent an i_lock/state_lock inversion, change nfsd4_init_file to use ihold instead if igrab. That's also more efficient anyway as we definitely hold a reference to the inode at that point. Signed-off-by:
-
Christoph Hellwig authored
nfsd4_process_open2 will currently will get access to the file, and then call nfsd4_truncate to (possibly) truncate it. If that operation fails though, then the access references will never be released as the nfs4_ol_stateid is never initialized. Fix by moving the nfsd4_truncate call into nfs4_get_vfs_file, ensuring that the refcounts are properly put if the truncate fails. Signed-off-by:
Christoph Hellwig <hch@infradead.org> Signed-off-by:
-
Kinglong Mee authored
fs/nfsd/nfs4xdr.c: In function 'nfsd4_encode_readv': >> fs/nfsd/nfs4xdr.c:3137:148: warning: comparison of distinct pointer types lacks a cast [enabled by default] thislen = min(len, ((void *)xdr->end - (void *)xdr->p)); Reported-by:
Fengguang Wu <fengguang.wu@intel.com> Signed-off-by:
-
J. Bruce Fields authored
Avoid an extra allocation for the tmpbuf struct itself, and stop ignoring some allocation failures. Signed-off-by: -
J. Bruce Fields authored
This is a not-that-useful kmalloc wrapper. And I'd like one of the callers to actually use something other than kmalloc. Signed-off-by: -
J. Bruce Fields authored
Signed-off-by: -
J. Bruce Fields authored
28e05dd8 "knfsd: nfsd4: represent nfsv4 acl with array instead of linked list" removed the last user that wanted a custom free function. Signed-off-by:
-
J. Bruce Fields authored
The name of a link is currently stored in cr_name and cr_namelen, and the content in cr_linkname and cr_linklen. That's confusing. Signed-off-by: -
J. Bruce Fields authored
Currently nfsd_symlink has a weird hack to serve callers who don't null-terminate symlink data: it looks ahead at the next byte to see if it's zero, and copies it to a new buffer to null-terminate if not. That means callers don't have to null-terminate, but they *do* have to ensure that the byte following the end of the data is theirs to read. That's a bit subtle, and the NFSv4 code actually got this wrong. So let's just throw out that code and let callers pass null-terminated strings; we've already fixed them to do that. Signed-off-by: -
J. Bruce Fields authored
It's simple enough for NFSv2 to null-terminate the symlink data. A bit weird (it depends on knowing that we've already read the following byte, which is either padding or part of the mode), but no worse than the conditional kstrdup it otherwise relies on in nfsd_symlink(). Signed-off-by: -
J. Bruce Fields authored
An NFS operation that creates a new symlink includes the symlink data, which is xdr-encoded as a length followed by the data plus 0 to 3 bytes of zero-padding as required to reach a 4-byte boundary. The vfs, on the other hand, wants null-terminated data. The simple way to handle this would be by copying the data into a newly allocated buffer with space for the final null. The current nfsd_symlink code tries to be more clever by skipping that step in the (likely) case where the byte following the string is already 0. But that assumes that the byte following the string is ours to look at. In fact, it might be the first byte of a page that we can't read, or of some object that another task might modify. Worse, the NFSv4 code tries to fix the problem by actually writing to that byte. In the NFSv2/v3 cases this actually appears to be safe: - nfs3svc_decode_symlinkargs explicitly null-terminates the data (after first checking its length and copying it to a new page). - NFSv2 limits symlinks to 1k. The buffer holding the rpc request is always at least a page, and the link data (and previous fields) have maximum lengths that prevent the request from reaching the end of a page. In the NFSv4 case the CREATE op is potentially just one part of a long compound so can end up on the end of a page if you're unlucky. The minimal fix here is to copy and null-terminate in the NFSv4 case. The nfsd_symlink() interface here seems too fragile, though. It should really either do the copy itself every time or just require a null-terminated string. Reported-by:
-
- 23 Jun, 2014 6 commits
-
-
Jeff Layton authored
Currently rpc_pton() fails to handle the case where you echo an address into the file, as it barfs on the newline. Ensure that we NULL out the first occurrence of any newline. Signed-off-by:
-
Jeff Layton authored
AFAICT, the only way to hit this error is to pass this function a bogus "who" value. In that case, we probably don't want to return -1 as that could get sent back to the client. Turn this into nfserr_serverfault, which is a more appropriate error for a server bug like this. Signed-off-by:
-
Jeff Layton authored
The filehandle structs all use host-endian values, but will sometimes stuff big-endian values into those fields. This is OK since these values are opaque to the client, but it confuses sparse. Add __force to make it clear that we are doing this intentionally. Signed-off-by:
-
Jeff Layton authored
The callers expect a __be32 return and the functions they call return __be32, so having these return int is just wrong. Also, nfsd_finish_read can be made static. Signed-off-by:
-
Jeff Layton authored
We currently hash the XID to determine a hash bucket to use for the reply cache entry, which is fed into hash_32 without byte-swapping it. Add __force to make sparse happy, and add some comments to explain why. Signed-off-by:
-
Jeff Layton authored
sparse complains that we're stuffing non-byte-swapped values into __be32's here. Since they're supposed to be opaque, it doesn't matter much. Just add __force to make sparse happy. Signed-off-by:
-
